Closed Bug 1370793 Opened 3 years ago Closed 3 years ago

stylo: mochitest crash - browser_CTP_favorfallback.js

Categories

(Core :: CSS Parsing and Computation, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: shinglyu, Assigned: heycam)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

[task 2017-06-07T03:45:39.153912Z] 03:45:39     INFO - TEST-START | browser/base/content/test/plugins/browser_CTP_favorfallback.js
[task 2017-06-07T03:45:39.444309Z] 03:45:39     INFO - GECKO(1487) | Redirecting call to abort() to mozalloc_abort
[task 2017-06-07T03:45:39.557619Z] 03:45:39     INFO - GECKO(1487) | ExceptionHandler::GenerateDump cloned child 1570
[task 2017-06-07T03:45:39.560776Z] 03:45:39     INFO - GECKO(1487) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2017-06-07T03:45:39.561302Z] 03:45:39     INFO - GECKO(1487) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2017-06-07T03:45:40.408047Z] 03:45:40     INFO - TEST-INFO | Main app process: exit 11
Priority: -- → P1
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8876472 - Flags: review?(bobbyholley)
Comment on attachment 8876471 [details]
Bug 1370793 - Part 1: Don't try to style unstyled children of elements with newly applied XBL bindings if in a display:none or unstyled subtree.

https://reviewboard.mozilla.org/r/147818/#review152454
Attachment #8876471 - Flags: review?(bobbyholley) → review+
Comment on attachment 8876472 [details]
geckolib: Don't panic when attempting to restyle an element with newly applied XBL bindings.

https://reviewboard.mozilla.org/r/147820/#review152460

::: servo/ports/geckolib/glue.rs:295
(Diff revision 1)
> -    element.has_dirty_descendants() || element.borrow_data().unwrap().has_restyle()
> +    restyle_behavior != Restyle::ForNewlyBoundElement &&
> +        (element.has_dirty_descendants() || element.borrow_data().unwrap().has_restyle())

I don't get what the point of this is. Why can't Gecko, which already knows that we're in the ForNewlyBoundElement case, just ignore the post-traversal required? Is the idea just that it makes more sense for that check to happen here rather than there?

I'm ambivalent about this, but if you think it really makes sense please document what's going on here, why the bit will be set, and why we don't want to do a post-traversal.
Attachment #8876472 - Flags: review?(bobbyholley) → review+
Comment on attachment 8876473 [details]
Bug 1370793 - Part 2: Crashtest.

https://reviewboard.mozilla.org/r/147822/#review152462
Attachment #8876473 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> I'm ambivalent about this, but if you think it really makes sense please
> document what's going on here, why the bit will be set, and why we don't
> want to do a post-traversal.

Oh, it's the element.borrow_data().unwrap() that panics in this case, since we skip restyling this element.  An alternative would be to do:

  element.has_dirty_descendants() || element.borrow_data().unwrap_or(false).has_restyle()

but it probably makes sense to keep panicking in cases we don't end up with some expected styles.
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #5)
> I'm ambivalent about this, but if you think it really makes sense please
> document what's going on here, why the bit will be set, and why we don't
> want to do a post-traversal.

Also, StyleNewlyBoundElement only ever styles previously-unstyled elements, so we shouldn't have any post-traversal work to do.  (We still can't assert that we return false, though, for the same reason we can't assert it in StyleNewChildren.)  I'll add a comment.
Attachment #8876472 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/938382694b20
Part 1: Don't try to style unstyled children of elements with newly applied XBL bindings if in a display:none or unstyled subtree. r=bholley
https://hg.mozilla.org/integration/autoland/rev/273b70c11163
Part 2: Crashtest. r=bholley
https://hg.mozilla.org/mozilla-central/rev/938382694b20
https://hg.mozilla.org/mozilla-central/rev/273b70c11163
https://hg.mozilla.org/mozilla-central/rev/b8a9bc890b3a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.