1370793 - stylo: mochitest crash - browser_CTP_favorfallback.js

Status: RESOLVED FIXED

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

Description

[Shing Lyu [:shinglyu]]

Attachment: test-linux64-stylo_opt-mochitest-browser-chrome-7.log

[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

Comment 1

[Cameron McCormack (:heycam)]

Attachment: 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.

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

Comment 2

[Cameron McCormack (:heycam)]

Attachment: geckolib: Don't panic when attempting to restyle an element with newly applied XBL bindings.

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

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1370793 - Part 2: Crashtest.

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

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.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8876473 [details]
Bug 1370793 - Part 2: Crashtest.

https://reviewboard.mozilla.org/r/147822/#review152462

Comment 7

[Cameron McCormack (:heycam)]

(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.

Comment 8

[Cameron McCormack (:heycam)]

(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.

Comment 9

[Cameron McCormack (:heycam)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147818/diff/1-2/

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8876473 [details]
Bug 1370793 - Part 2: Crashtest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/147822/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8a9bc890b3a
Followup bustage fix. r=me

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/938382694b20
https://hg.mozilla.org/mozilla-central/rev/273b70c11163
https://hg.mozilla.org/mozilla-central/rev/b8a9bc890b3a