Closed Bug 1441613 Opened 4 years ago Closed 4 years ago

Crash in mozilla::dom::GetFontStyleForServo

Categories

(Core :: CSS Parsing and Computation, defect)

Unspecified
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- fixed

People

(Reporter: marcia, Assigned: emilio)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(3 files)

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 

=============================================================
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?
Flags: needinfo?(emilio)
I... I cannot make any sense of what that code is trying to assert.
Assignee: nobody → emilio
Flags: needinfo?(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

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
Attachment #8954689 - Flags: review?(hikezoe) → review+
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 :-)
Actually I had a pretty simple testcase for this.
Attached file testcase
(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!
For this to be reliable you may need to add reftest-wait, I guess.
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/020a45f8ebe7
Properly check for shell destruction instead of just nonsensically assert. r=hiro
Attached patch Crashtest.Splinter Review
I'll try to push it whenever the tree is open again, feel free to do it if I miss it.
Pushed by ecoal95@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/020a45f8ebe7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.