1441613 - Crash in mozilla::dom::GetFontStyleForServo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-f22bddc2-6435-4491-a67b-f00170180227.
=============================================================

Seen while looking at nightly crash stats - 61 crashes/48 installations and appears to have started in 20180225220119: http://bit.ly/2F8ETIp

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b3191953ccdabd335ffb383905a36b0062e547b2&tochange=7208b6a7b11c3ed8c87a7f17c9c30a8f9583e791

One comment (used Google Translate) When a Google ad appears in the video footer the flap locks

Top 6 frames of crashing thread:

0 xul.dll mozilla::dom::GetFontStyleForServo dom/canvas/CanvasRenderingContext2D.cpp:2934
1 xul.dll mozilla::dom::CanvasRenderingContext2D::SetFontInternal dom/canvas/CanvasRenderingContext2D.cpp:4006
2 xul.dll mozilla::dom::CanvasRenderingContext2DBinding::set_font dom/bindings/CanvasRenderingContext2DBinding.cpp:6222
3 xul.dll mozilla::dom::GenericBindingSetter dom/bindings/BindingUtils.cpp:2992
4 xul.dll js::jit::CallNativeSetter js/src/jit/VMFunctions.cpp:1572
5  @0x60815965bf 

=============================================================

Comment 1

[Julien Cristau [:jcristau]]

These are MOZ_RELEASE_ASSERT(parentStyle) (Should have a valid parent style) from
https://hg.mozilla.org/mozilla-central/file/bd6e200b5a6b2f9e5a9b31fcc92285aa7fc9afcc/dom/canvas/CanvasRenderingContext2D.cpp#l2934

emilio, does this ring a bell?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I... I cannot make any sense of what that code is trying to assert.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1441613: Properly check for shell destruction instead of just nonsensically assert.

GetStyleContext can flush. As such, that flush can kill the pres shell, and the
return value could be null. I have no idea why that code was asserting it didn't
happen, but that assert is completely bogus.

Throw instead, just like GetFontParentStyleContext used to do for Gecko.

Review commit: https://reviewboard.mozilla.org/r/223802/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/223802/

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8954689 [details]
Bug 1441613: Properly check for shell destruction instead of just nonsensically assert.

https://reviewboard.mozilla.org/r/223802/#review229820

Thanks!
I don't recall why I didn't originally put early return after FlushPendingNotifications[1].

[1] https://hg.mozilla.org/mozilla-central/rev/1876d89c8f37fb06dd2e20bee33f69333565b669#l1.67

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8954689 [details]
Bug 1441613: Properly check for shell destruction instead of just nonsensically assert.

https://reviewboard.mozilla.org/r/223802/#review229820

Oh, so fun how much the code has changed since that assert was introduced :-)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Actually I had a pretty simple testcase for this.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: testcase

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> Created attachment 8954706 [details]
> testcase

I wouldn't have expected it to be so straight-forward, but makes sense of course. Will land as a crashtest, with a commit to your name of course.

Thanks!

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

For this to be reliable you may need to add reftest-wait, I guess.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/020a45f8ebe7
Properly check for shell destruction instead of just nonsensically assert. r=hiro

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Crashtest.

I'll try to push it whenever the tree is open again, feel free to do it if I miss it.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c023fae29ccc
Crashtest. r=emilio

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d4b47e577288
Fixup crashtest so that it actually removes the reftest-wait class. r=me on a CLOSED TREE

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/020a45f8ebe7

Comment 15

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/c023fae29ccc
https://hg.mozilla.org/mozilla-central/rev/d4b47e577288