1394586 - stylo: panicked at '<img> has still dirty bit true or animation-only dirty bit true'

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

Attachment: log

export STYLO_FORCE_ENABLED=1
export RUST_BACKTRACE=1

https://nokia-software-updater.en.softonic.com/#app-softonic-review

thread '<unnamed>' panicked at '<img> (0x7f233622dbc0) has still dirty bit true or animation-only dirty bit true', /mozilla/builds/nightly/mozilla/servo/ports/geckolib/glue.rs:3311
stack backtrace:
   0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
             at /checkout/src/libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at /checkout/src/libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at /checkout/src/libstd/sys_common/backtrace.rs:60
             at /checkout/src/libstd/panicking.rs:355
   3: std::panicking::default_hook
             at /checkout/src/libstd/panicking.rs:371
   4: std::panicking::rust_panic_with_hook
             at /checkout/src/libstd/panicking.rs:549
   5: std::panicking::begin_panic
             at /checkout/src/libstd/panicking.rs:511
   6: std::panicking::begin_panic_fmt
             at /checkout/src/libstd/panicking.rs:495
   7: geckoservo::glue::Servo_AssertTreeIsClean::assert_subtree_is_clean
             at /mozilla/builds/nightly/mozilla/servo/ports/geckolib/glue.rs:3311

reproduced on Linux x64 from https://hg.mozilla.org/mozilla-central/rev/e2efa420beb1a578c7350ba925c82230da6b1267

This is with a build that includes the patch from Bug 1389645 - stylo: panicked at '<div> has still dirty bit true or animation-only dirty bit false'

Comment 1

[Emilio Cobos Álvarez (:emilio)]

rr is awesome, finding this without it would've been impossible.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1394586: Unconditionally clear the restyle flags on Element::BindToTree.

The failure here is a <script> that document.writes a whole HTML document, which
happens to have the Gecko style back-end.

Of all the elements written there, there's an <img> element which triggers a
load, which finishes and changes the state of the <img>, posting a restyle event
to its Gecko-backed restyle manager.

At that point, but before the refresh driver ticks, the element is unbound from
the tree, keeping the restyle root bits from the Gecko back-end.

Afterwards, the element is bound to a Servo-backed document, so
ClearRestyleFlagsIfGecko does nothing (since the document pointer is already
set, and points to a Servo-backed document).

Furthermore, it's inserted on a display: none subtree, so we never reach it
during the traversal to remove the flags.

Other alternative fixes for this same issue would be:

 * Clearing the Gecko restyle flags in UnbindFromTree. Gecko doesn't seem to
   prevent restyle roots from being outside of the document (it just does
   nothing when finding them on ProcessPendingRestyles), so it doesn't seem a
   very reliable solution.

 * Reorder the flag clearing to be before setting the document in BindToTree.
   Seems somewhat risky, and for no good reason.

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

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8902263 [details]
Bug 1394586: Unconditionally clear the restyle flags on Element::BindToTree.

https://reviewboard.mozilla.org/r/173794/#review179126

::: dom/base/Element.cpp:1611
(Diff revision 1)
> -    // And the restyle bits
> -    UnsetRestyleFlagsIfGecko();
> +               // And the restyle bits. These shouldn't even get set if we came
> +               // from a Servo-styled document, but they may be set if the
> +               // element comes from a Gecko-backed document, see bug 1394586.

Expand this comment a bit to say that we should be able to remove this when we drop support for the old style system.

::: dom/base/Element.cpp:1622
(Diff revision 1)
> -    UnsetFlags(NODE_FORCE_XBL_BINDINGS |
> -               NODE_NEEDS_FRAME | NODE_DESCENDANTS_NEED_FRAMES);
> +    UnsetFlags(NODE_FORCE_XBL_BINDINGS | NODE_NEEDS_FRAME |
> +               NODE_DESCENDANTS_NEED_FRAMES | ELEMENT_ALL_RESTYLE_FLAGS);

Reference the other comment from here.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8902263 [details]
Bug 1394586: Unconditionally clear the restyle flags on Element::BindToTree.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5735af08cfef
Unconditionally clear the restyle flags on Element::BindToTree. r=bholley

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5735af08cfef