New issue
Advanced search Search tips

Issue 846708 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Null-dereference READ in blink::Editor::FindString

Project Member Reported by ClusterFuzz, May 25 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=6309226230841344

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000520
Crash State:
  blink::Editor::FindString
  blink::LocalDOMWindow::find
  blink::V8Window::findMethodCallback
  
Sanitizer: thread (TSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=561767:561768

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6309226230841344

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Project Member

Comment 1 by ClusterFuzz, May 25 2018

Components: Blink>Editing Blink>Internals
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, May 25 2018

Labels: Test-Predator-Auto-Owner
Owner: ekaramad@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/596e6b3281a8cc2c4b646553e4cde169a29a1d1d (Cleanup plugin element frames when the layout tree is detached).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
Cc: dcheng@chromium.org
Components: Blink>HTML>Object
Status: Started (was: Assigned)
The CL is the culprit. 

IIUC, the issue is due to detaching frame during InStyleRecalc() phase. I have a PoC CL+test here:
https://chromium-review.googlesource.com/c/chromium/src/+/1073204

A short test case based on clusterfuzz sample is in the CL (triggers a DCHECK).
 
The PoC CL fixes this,  issue 846703 , and the test case.

Danielm WDYT? Does this make sense, and, is there any other "phases" we should consider before detaching the frame?

Cc: ekaramad@chromium.org
 Issue 846703  has been merged into this issue.
Project Member

Comment 5 by ClusterFuzz, May 26 2018

Labels: OS-Windows
 Issue 846969  has been merged into this issue.
 Issue 846931  has been merged into this issue.
 Issue 846933  has been merged into this issue.
 Issue 846978  has been merged into this issue.
Project Member

Comment 10 by ClusterFuzz, May 27 2018

Labels: OS-Chrome
Components: -Blink>Editing
 Issue 847042  has been merged into this issue.
 Issue 847062  has been merged into this issue.
 Issue 847084  has been merged into this issue.
 Issue 847232  has been merged into this issue.
 Issue 847320  has been merged into this issue.
Project Member

Comment 17 by ClusterFuzz, May 29 2018

Labels: OS-Mac
Project Member

Comment 18 by ClusterFuzz, May 29 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 5560575313313792 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Status: Started (was: Verified)
Some of the merged bugs including this one still reproduce.
Cc: yosin@chromium.org
 Issue 847949  has been merged into this issue.
 Issue 848785  has been merged into this issue.
Project Member

Comment 22 by bugdroid1@chromium.org, Jun 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/815589c705ec4f012c42cc3cd5e98519227b17e4

commit 815589c705ec4f012c42cc3cd5e98519227b17e4
Author: Ehsan Karamad <ekaramad@chromium.org>
Date: Sat Jun 02 01:33:34 2018

Revert "Cleanup plugin element frames when the layout tree is detached"

This reverts commit 596e6b3281a8cc2c4b646553e4cde169a29a1d1d.

Reason for revert: DetachLayoutTree could be called at inopportune times
such as style recalc or during layout. This caused several crashes.

Original change's description:
> Cleanup plugin element frames when the layout tree is detached
>
> Unlike frames, plugin elements (<embed> and <object>) are updated
> when their layout tree is rebuilt. However, due to an oversight,
> <embed> or <object> elements displaying subframes would only have
> the associated content view (FrameView) torn down, leaving a
> dangling content frame.
>
> This led to some interesting side effects:
> - When transitioning from a local frame to a plugin, the content
>   frame would remain live, with JS still happily running.
> - When transitioning from a remote frame to a plugin,
> - When navigating a remote frame from one URL to another, the
>   element would stop updating since the FrameView would be torn
>   down but a new one would never be created. Note that this was
>   not (as much of) a problem for local frames, since local frames
>   re-create the LocalFrameView on every navigation.
> With this CL, if a plugin element has an associated content frame,
> use that to clean up the state associated with the element when the
> layout tree is detached. This can cause the browser context to be
> re-created: this matches the behavior of Edge and Firefox.
>
> Note that there is still one edge case where Blink behaves oddly:
> if the navigation fails and should display fallback content, the
> content frame still remains attached. This will be addressed in
> followups.
>
> Link to whatwg issue: https://github.com/whatwg/html/issues/3576
>
> Bug: 776510, 781880
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
> Change-Id: Id18605fbe602ea6c0076c1d579345cdcf28cc984
> Reviewed-on: https://chromium-review.googlesource.com/996314
> Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#561768}

TBR=dcheng@chromium.org,creis@chromium.org,alexmos@chromium.org,ekaramad@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 776510, 781880, 847216,  846708 
Change-Id: I762631b92463352bea9bf3102d90a13b72776786
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Reviewed-on: https://chromium-review.googlesource.com/1083500
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Ehsan Karamad <ekaramad@chromium.org>
Commit-Queue: Ehsan Karamad <ekaramad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563902}
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/content/browser/site_per_process_browsertest.cc
[delete] https://crrev.com/5b3021d75ad1bca4408f05c566de4a92793514f7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/resources/test_page.html
[delete] https://crrev.com/5b3021d75ad1bca4408f05c566de4a92793514f7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-embed-element/detach-frame-on-src-change.html
[delete] https://crrev.com/5b3021d75ad1bca4408f05c566de4a92793514f7/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/the-object-element/detach-frame-on-data-change.html
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/WebKit/LayoutTests/http/tests/security/xss-DENIED-object-element.html
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/blink/renderer/core/exported/web_frame_test.cc
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/blink/renderer/core/frame/frame_test_helpers.cc
[modify] https://crrev.com/815589c705ec4f012c42cc3cd5e98519227b17e4/third_party/blink/renderer/core/html/html_plugin_element.cc

Project Member

Comment 23 by ClusterFuzz, Jun 2 2018

ClusterFuzz has detected this issue as fixed in range 563898:563909.

Detailed report: https://clusterfuzz.com/testcase?key=6309226230841344

Fuzzer: ifratric-browserfuzzer-v3
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000520
Crash State:
  blink::Editor::FindString
  blink::LocalDOMWindow::find
  blink::V8Window::findMethodCallback
  
Sanitizer: thread (TSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=561767:561768
Fixed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=563898:563909

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6309226230841344

See https://github.com/google/clusterfuzz-tools for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Status: Fixed (was: Started)
Following comments #22 & #23 marking as fixed.

Sign in to add a comment