New issue
Advanced search Search tips

Issue 870873 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 21
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 1
Type: Bug



Sign in to add a comment

Null-dereference READ in blink::Canvas2DLayerBridge::Hibernate

Project Member Reported by ClusterFuzz, Aug 3

Issue description

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

Fuzzer: inferno_twister_c
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: Null-dereference READ
Crash Address: 0x000000000000
Crash State:
  blink::Canvas2DLayerBridge::Hibernate
  base::internal::Invoker<base::internal::BindState<void
  blink::scheduler::MainThreadSchedulerImpl::RunIdleTask
  
Sanitizer: address (ASAN)

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

Issue filed automatically.

See https://github.com/google/clusterfuzz-tools for more information.
 
Cc: kkaluri@chromium.org
Labels: M-69 Test-Predator-Wrong
Owner: jbroman@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL could not provide any possible suspects.

Using Code Search for the file, "main_thread_scheduler_impl.cc" suspecting the below Cl might have caused this issue

Suspect CL: https://chromium.googlesource.com/chromium/src/+/da7734ae3729755b6c2b1074e488c83bd1f98092

jbroman@ -- Could you please check whether this is caused with respect to your change, if not please help us in assigning it to the right owner.

Thanks!
Cc: fs...@chromium.org
Components: Blink>Canvas
Owner: ----
Status: Untriaged (was: Assigned)
I don't see how my patch changing the type of an unused parameter could have caused this. Not able to reproduce locally, and not seeing any other likely culprits, bouncing to canvas team for triage.
Labels: CF-NeedsTriage
Unable to find actual suspect through code search and also observing no CL's under regression range, hence adding appropriate label and requesting someone from dev team to look in to this issue.

Owner: fs...@chromium.org
Status: Assigned (was: Untriaged)
fserb@, this has passed our 7-day triage window. Could you please look at it an re-triage?
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 14

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

commit b34b7be96c8e4026ab5f17598b4c0682b70b532f
Author: Miguel Casas <mcasas@chromium.org>
Date: Tue Aug 14 14:47:21 2018

lowLatency canvas2D: fix hibernation tab crash

lowLatency 2D contexts would crash the tab when sent to the background;
this is because of the callstack
  HTMLCanvasElement::PageVisibilityChanged()
   CanvasRenderingContext2D::SetIsHidden()
     canvas()->GetCanvas2DLayerBridge()->SetIsHidden(hidden)
     ...
       Canvas2DLayerBridge::Hibernate()

and that tries to destroy its Layer, which has not been created
(because in lowLatency mode we don't use it).

This CL fixes the issue by cutting at SetIsHidden() if there is
no |layer_|.

Test: open https://codepen.io/miguelao/full/ZjJNNw (or any other
trivial sample canvas lowLatency demo), draw something , then
open a new tab. The former tab crashes on ToT, doesn't with this CL.

Bug: 839970,  870873 
Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6657a72009438cb31fac2b2d874c807b1719d16a
Reviewed-on: https://chromium-review.googlesource.com/1173344
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582913}
[modify] https://crrev.com/b34b7be96c8e4026ab5f17598b4c0682b70b532f/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc
[modify] https://crrev.com/b34b7be96c8e4026ab5f17598b4c0682b70b532f/third_party/blink/renderer/platform/graphics/canvas_2d_layer_bridge.cc

Labels: Merge-Request-69
Status: Started (was: Assigned)
Project Member

Comment 7 by sheriffbot@chromium.org, Aug 21

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: We are only 13 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
How safe is the change to merge to M69? And why it is critical to merge to M69 this late in release cycle? 
it's super safe (just a null check before using a variable).
We got a clusterfuzz crash on this as well as a ChromeOS Hibernation crash. I think we should do it.

Comment 10 Deleted

Labels: -Merge-Approved-69 Merge-Rejected-69
Rejecting merge to M69 as it is not must merge per chat with fserb@.
Status: Fixed (was: Started)
Ok. We are dropping this for M-69, but it's fixed on M70.
Project Member

Comment 13 by ClusterFuzz, Aug 28

Labels: Needs-Feedback
ClusterFuzz testcase 4542402992537600 is still reproducing on tip-of-tree build (trunk).

Please re-test your fix against this testcase and if the fix was incorrect or incomplete, please re-open the bug. Otherwise, ignore this notification and add ClusterFuzz-Wrong label.

Sign in to add a comment