1214571 - Firefox Nightly 44.0a1 (2015-10-12) crashes in mozilla::dom::HTMLCanvasElement

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Crashes happens mostly when you close the Firefox, but it can also happens when you swap the tab to another one.


[Tracking Requested - why for this release]: Regression


Regression window (mozilla-central)
Good:
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-10-11-03-02-29-mozilla-central/

Bad:
https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-10-12-03-06-12-mozilla-central/

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b68eab795f9de072bee12821b0f09422e5aa0da9&tochange=0b69d304f861d0038fb78f1d52b0f5d13ef7c6fe

Crashlog reports [@ mozilla::dom::HTMLCanvasElement::OnVisibilityChange() ]:
https://crash-stats.mozilla.com/report/index/d75adabe-5159-4d82-8aa9-df02d2151012
https://crash-stats.mozilla.com/report/index/ff9a1d5c-0db5-4d5f-a381-9c3c32151012
https://crash-stats.mozilla.com/report/index/22f8f058-1081-477a-8451-78bdd2151013
https://crash-stats.mozilla.com/report/index/db80e051-041b-4475-9b06-c9ef42151013
https://crash-stats.mozilla.com/report/index/10390cd6-af7e-4103-b506-7b1272151013
https://crash-stats.mozilla.com/report/index/2c865c8a-7ca1-40d1-b1d4-1f2282151013

Crashlog reports [@ mozilla::dom::HTMLCanvasElementObserver::UnregisterVisibilityChangeEvent() ];
https://crash-stats.mozilla.com/report/index/33b2cfea-1f4b-4cf0-a51a-fccee2151012
https://crash-stats.mozilla.com/report/index/b7983fe3-5458-4b26-b2d0-2c8b02151012
https://crash-stats.mozilla.com/report/index/f8ca6fc4-a371-49bc-bad7-d90dd2151013

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Based on the crash stats, we have somehow missed to call
HTMLCanvasElementObserver::Destroy() since HTMLCanvasElementObserver::HandleEvent calls OnVisibilityChange() on using a deleted mElement, if I read the stacks right.

Raw pointer as a member variable bites again.
HTMLCanvasElementObserver has HTMLCanvasElement* mElement;

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Btw, looks like it is possible that we create several HTMLCanvasElementObserver objects, but call
Destroy on only one of them.

Comment 3

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

I also want to mention that I have these option set as "false" in about:config:
-webgl.angle.try-d3d11
-webgl.can-lose-context-in-foreground
-webgl.enable-debug-renderer-info
-webgl.restore-context-when-visible
and these to "true:
-webgl.disable-angle
-webgl.disable-extensions
-webgl.disable-fail-if-major-performance-caveat
-webgl.disabled
to disable completely WebGL per security reasons and per not using it.

Comment 4

[Andrea Marchesini [:baku]]

(In reply to Olli Pettay [:smaug] from comment #2)
> Btw, looks like it is possible that we create several
> HTMLCanvasElementObserver objects, but call
> Destroy on only one of them.

HTMLCanvasElementObserver calls Destroy in its DTOR. But yes, we should check if we already have an existing mContextObserver.

Comment 5

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Raw pointer as a member variable bites again.
> HTMLCanvasElementObserver has HTMLCanvasElement* mElement;

Sigh.  :(  I should land my analysis soon...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

Not 100% reliable testcase, since it depends on CC/GC scheduling, but seems to crash locally usually in HTMLCanvasElement::OnVisibilityChange()
But hopefully it helps figuring out the right patch for this.

c.getContext("webgl", { get stencil() { throw "hahaa"; } }); creates 
an HTMLCanvasElementObserver which c.getContext("webgl", { stencil: false }); then overrides.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Only create mContextObserver once.

Sorry for crashing nightly. Here is patch to prevent create too much mContextObserver.

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc9f8e255e2d

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7828bf225ef2

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7828bf225ef2

Comment 11

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Verified as FIXED with (2015-10-17) build.
Thank you very much. \o/

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Can someone suggest a security rating for this issue?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

See the change after https://bugzilla.mozilla.org/show_bug.cgi?id=1214571#c1
sec-critical because of accessing a deleted object.