Closed Bug 1313884 Opened 4 years ago Closed 3 years ago

Crash in mozilla::dom::CanvasRenderingContext2D::DrawImage

Categories

(Core :: Canvas: 2D, defect, P3, critical)

50 Branch
defect

Tracking

()

RESOLVED DUPLICATE of bug 1318283
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: philipp, Assigned: rhunt)

References

Details

(Keywords: crash, regression, Whiteboard: [gfx-noted])

Crash Data

Attachments

(2 files, 2 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-2c5b1fbf-d548-417a-acbe-f86d52161029.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&) 	dom/canvas/CanvasRenderingContext2D.cpp:4847
1 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, mozilla::ErrorResult&) 	obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:218
2 	xul.dll 	mozilla::dom::CanvasRenderingContext2DBinding::drawImage 	obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:4176
3 	xul.dll 	mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) 	dom/bindings/BindingUtils.cpp:2770
4 		@0x3e560ca7 	
5 		@0xffffff8b

this is a cross-platform crash, which has been around for a while, but is regressing in numbers since firefox 50 beta builds.
overall it's still at a rather low volume though (0.05% of browser crashes on 50.0b last week).
In this place, we're assuming AdjustTarget::mTarget is not null, though it can be (and we check elsewhere in the code).  May as well check.
I would like to understand why this spiked up though.
Assignee: nobody → rhunt
Priority: -- → P3
Whiteboard: [gfx-noted]
Attached patch check-mtarget.patch (obsolete) — Splinter Review
This patch mirrors other places in CanvasRenderingContext2D that check IsTargetValid() before using mTarget. This should make sure that EnsureTarget() succeeded and mTarget is valid.

There are quite a few other places that don't do this, and I'm not sure if that is a problem.
Comment on attachment 8806594 [details] [diff] [review]
check-mtarget.patch

Review of attachment 8806594 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +4720,5 @@
>  
>    Element* element = nullptr;
>  
>    EnsureTarget();
> +  if (!IsTargetValid()) {

If we crash today when the target is not valid, let's put a gfxCriticalError() here, so that we get an assert or a note in the crash report.
Attachment #8806594 - Attachment is obsolete: true
Attachment #8806806 - Flags: review?(milan)
Attachment #8806806 - Flags: review?(milan) → review+
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ae634cfdf2
Check if mTarget is null in CanvasRenderingContext2D::DrawImage
https://hg.mozilla.org/mozilla-central/rev/c7ae634cfdf2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
hi, this looks like a simple null-check - do you think we can uplift this to 50/51 in terms of scope and risk?
Flags: needinfo?(milan)
We don't know what caused the crash to spike, so I will check after the weekend if this has done the job, or just moved the crash elsewhere, and request the uplift as appropriate.
This patch didn't quite stop the crash: https://crash-stats.mozilla.com/report/index/1388d08f-bd97-4528-97d0-429c82161104
Status: RESOLVED → REOPENED
Flags: needinfo?(milan) → needinfo?(rhunt)
Resolution: FIXED → ---
Flags: needinfo?(milan)
I am not sure what is going on here. My best guess is that somehow IsValidTarget() is returning true even when mTarget is null.

Looking at the code for IsValidTarget() this is technically possible if sErrorTarget == null && mBufferProvider != null. But I can't find a case where this could happen.

Milan, would it make sense to put a release assert in AdjustedTarget and other locations to see if that case is happening and when? 

If it isn't that mTarget is null, then something else is and I can't identify another good possibility.
Flags: needinfo?(rhunt)
A quick patch with what I had in mind - check for the null pointer, get some more information and crash in the cases where bad things happen, but only in nightly+aurora.
Flags: needinfo?(milan)
Attachment #8809490 - Flags: review?(gwright)
Attachment #8809490 - Flags: review?(gwright) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4879b87c17
Part 2: Additional nullptr test and nightly/aurora crash if bad things happen. r=gw280
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/056085427582d32c24ae55d1c5248c31fd6a5fc1 - crashing when bad things happen is one thing, but saying that "bad things have happened" every time we run reftests, https://treeherder.mozilla.org/logviewer.html#?job_id=39033633&repo=mozilla-inbound, or crashtests, https://treeherder.mozilla.org/logviewer.html#?job_id=39033719&repo=mozilla-inbound, or mochitests, https://treeherder.mozilla.org/logviewer.html#?job_id=39035519&repo=mozilla-inbound, or, or, or, or, is another thing entirely. Even I'm not so pessimistic as to say that bad things happen almost every time we run a test.
Thanks for backing this out.  While getting all these failures is great, I will remain ashamed for another 24 hours for not pushing this to try in the first place.
(In reply to Milan Sreckovic [:milan] from comment #15)
> Created attachment 8809875 [details] [diff] [review]
> Check for null pointer, diagnostic crash in nightly+aurora. r=jrmuizel
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09175243e5029b4e9f7ea6a840157fe99a40eac7

Based on Jeff's suggestion as to how to work around the lifetime problem in the previous patch.
(In reply to Milan Sreckovic [:milan] from comment #15)
> Created attachment 8809875 [details] [diff] [review]
> Check for null pointer, diagnostic crash in nightly+aurora. r=jrmuizel
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=09175243e5029b4e9f7ea6a840157fe99a40eac7

Green.
Attachment #8809875 - Flags: review?(jmuizelaar) → review+
(In reply to Joel Maher ( :jmaher) from comment #18)
> this had a regression and when backed out an improvement for windows xp svg
> opacity:
> https://treeherder.mozilla.org/perf.html#/graphs?series=%5Bmozilla-inbound,
> 271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,1,1%5D&series=%5Bautoland,
> 271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,1,1%5D&zoom=1478775369376.7124,
> 1478890356458.904,523.3382906435147,557.0185880412843&selected=%5Bmozilla-
> inbound,271bc618d6e4dce8d7daae1130bc8fdada3dfaf6,143829,39033623,1%5D

Interesting - it was a UAF scenario, so let's see what the "real" patch does.
Keywords: checkin-needed
Target Milestone: mozilla52 → ---
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/741ff57b2407
Part 2. Additional nullptr test and nightly/aurora crash if bad things happen. r=gw280
Keywords: checkin-needed
(In reply to Pulsebot from comment #20)
> Pushed by msreckovic@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/741ff57b2407
> Part 2. Additional nullptr test and nightly/aurora crash if bad things
> happen. r=gw280

Looks like I lied in the comments as to who reviewed this.
http://bit.ly/2fwvPQm seems to have also spiked, not sure if it needs it's own bug or is covered under this one.
I imagine the same underlying cause, and we can make sure we fix it all the same way.  Once the second patch lands we may get more information and understand what the cause is.
Seems to be related to filters.
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1318283
Depends on: 1334647
Depends on: 1316866
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.