Closed Bug 1383981 Opened 7 years ago Closed 7 years ago

Stylo: Robohornet CSS selectors test OOMs

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: ajones, Assigned: emilio)

References

(Blocks 2 open bugs, )

Details

Crash Data

Attachments

(2 files, 1 obsolete file)

Steps to reproduce:

1. Enable Stylo
2. Navigate to http://www.robohornet.org/#et=css_selectors
3. Click on "Run the test"

Expected results:

Test to complete within about 5 seconds

Actual results:

Memory climbs until after a minute or so reaching 24GB and then OOMing.
Priority: -- → P1
I see what's going on.
Assignee: nobody → emilio+bugs
Comment on attachment 8889775 [details]
style: Avoid exponential blowup when processing invalidations of the same kind.

https://reviewboard.mozilla.org/r/160856/#review166116

::: servo/components/style/invalidation/element/invalidator.rs:507
(Diff revision 1)
> +            // TODO(emilio): The assumption here is that this contains() check
> +            // is cheap because the invalidation vector is usually slow. If this
> +            // if false, we can always do the same we do for sibling
> +            // invalidations and preemptively clone, then remove the
> +            // non-effective for the next level.
> +            if result.effective_for_next &&
> +               !descendant_invalidations.contains(invalidation) {

Do we have any microbenchmarks written for a high number of invalidations encountered?  I worry that it doesn't take that many entries in descendant_invalidations before we'll take a noticeable amount of time here, given the quadratic time behaviour.
Attachment #8889775 - Flags: review?(cam) → review+
I'll just do the hard fix, since I've noticed this is also broken for sibling combinators in the same way.
Flags: needinfo?(cam)
Comment on attachment 8889775 [details]
style: Avoid exponential blowup when processing invalidations of the same kind.

https://reviewboard.mozilla.org/r/160856/#review166638

::: servo/components/style/invalidation/element/invalidator.rs:639
(Diff revision 2)
> +                let can_skip_pushing =
> +                    next_invalidation_kind == invalidation_kind &&
> +                    invalidation.matched_by_any_previous &&
> +                    next_invalidation.effective_for_next();

So in the end I don't understand from reading the code, which means it needs comments explaining.  Can you give a small example of what's going on here?  In particular, why is the can_skip_pushing expression what it is?
Attachment #8889775 - Flags: review+ → review?(cam)
Flags: needinfo?(cam) → needinfo?(emilio+bugs)
I added a big comment that should cover the reason why the three conditions are needed, hopefully that'll make it, let me know if it doesn't and I can try to elaborate more I guess.
Flags: needinfo?(emilio+bugs)
We discussed on IRC and went through it, and thought that this should have a comment with an example to make it easier to reason about, so I did that.
Comment on attachment 8889775 [details]
style: Avoid exponential blowup when processing invalidations of the same kind.

https://reviewboard.mozilla.org/r/160856/#review167172

Thanks. :-)
Attachment #8889775 - Flags: review?(cam) → review+
Comment on attachment 8889817 [details]
Bug 1383981: Similar crashtest for sibling combinators.

https://reviewboard.mozilla.org/r/160906/#review167174
Attachment #8889817 - Flags: review?(cam) → review+
Attachment #8889775 - Attachment is obsolete: true
The Servo changes landed in https://github.com/servo/servo/pull/17888, fwiw.
https://hg.mozilla.org/mozilla-central/rev/6f13b6990f42
https://hg.mozilla.org/mozilla-central/rev/4cc438fa2205
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
How arbitrary is the depth 1000 here? Does it come from RoboHornet? How much smaller can we make it for this test still to serve its purpose as reflecting content that Firefox needs to deal with?

Context: bug 256180 comment 102.
Flags: needinfo?(emilio)
It was taken from robohornet, yeah. I guess you can always revert the servo patch and see when the blowup makes the browser crash, but I don't have better ideas really about how small it can be.
Flags: needinfo?(emilio)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #28)
> It was taken from robohornet, yeah.

Thanks. It looks like it's hard to simultaneously fix bug 256180 in a Chrome-compatible way and avoid making RoboHornet (and the crashtest added here) crash on Android without fixing bug 1400811 not only on aarch64 but on armv7, too.

(In reply to Henri Sivonen (:hsivonen) from comment #27)

How arbitrary is the depth 1000 here? Does it come from RoboHornet? How much
smaller can we make it for this test still to serve its purpose as
reflecting content that Firefox needs to deal with?

Note, the crashtest here occasionally triggers stack-overflows in automation: see bug 1825414 (and dupes bug 1866878 and bug 1868113 that I just triaged&duped for recent instances of the same crash). In one log that I looked at, we've got a backtrace that's 3672 levels deep when we crash.

Not sure if that's surprising or not, but we might want to consider reducing the 1000 depth if that's expected to be directly-proportional to the number of callstack levels.

You need to log in before you can comment on or make changes to this bug.