1442126 - Intermittent layout/style/test/test_load_events_on_stylesheets.html | Load event firing on broken stylesheet

Status: RESOLVED FIXED

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

Description

[Treeherder Bug Filer]

Filed by: csabou [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=165098535&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/Nf1GQTV1RPOFUSj-2N2dGA/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This smells a lot like something that could be caused by the recent work by Bobby has done... From a quick look I haven't figured out why though.

Bobby mind taking a look?

Comment 3

[Bobby Holley (:bholley)]

I ran this in rr chaos for an hour and caught a trace. Just need to debug it, which may or may not happen today.

Comment 4

[Intermittent Failures Robot]

13 failures in 831 pushes (0.016 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 4
* mozilla-inbound: 3
* mozilla-central: 2
* mozilla-beta: 2
* autoland: 2

Platform breakdown:
* linux64: 8
* linux32: 2
* linux64-qr: 1
* linux64-devedition: 1
* linux32-nightly: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1442126&startday=2018-02-26&endday=2018-03-04&tree=all

Comment 5

[Bobby Holley (:bholley)]

So, the rr trace shows some interesting racey behavior. :-)

The testcase loads example.com multiple times as an invalid stylesheet over the course of the test. Since we coalesce loads using the linked list on SheetLoadData, the failure for the import load can end up getting handled midway through the parse of the parent (which wouldn't happen otherwise, given event loop ordering).

So we end up here, after several iterations of the while loop (i.e. traversals down the linked list):

https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/layout/style/Loader.cpp#1920

mIsBeingParsed on the parent is true, so we bail out. But this means that the failure of the child load gets dropped on the floor, and we subsequently erroneously fire onload instead of onerror on the parent sheet when it finishes parsing.

I added the mIsBeingParsed thing. But it seems like the same behavior would happen if we had multiple child loads, since then mPendingChildren would be nonzero and we'd similarly drop the failure on the floor.

Boris, how is this supposed to work?

Comment 6

[Boris Zbarsky [:bzbarsky]]

That sounds really familiar... I was sure we had a bug on this, but I'm not finding it right now.

You are quite correct that right now the "status" of a sheet load is the status of the last import inside that sheet that completes and that this means that it can randomly be success or failure depending on how those loads race each other, if some of them succeed and some fail.

There's no spec for what the behavior should be here, of course.  I tried figuring out what browsers actually do... and it's a complete mess.  See https://github.com/whatwg/html/issues/3532

I think there are only two principled ways to handle this:

1)  Child load errors do not affect error state of parent at all.
2)  We store the load status in the SheetLoadData, child loads update that status
    if it's success and they fail, we use that status to decide whether to fire
    load or error.

I guess if we implement #2 then switching to #1 would be pretty simple if we decide to...  And conceptually, I like #2 more.

Comment 7

[Bobby Holley (:bholley)]

Boris and I discussed, sounds like he's going to write the patch and I'll review.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to consistently fail a sheet load if any of its imports fail

This fixes a race where we would fail if and only if our last-to-complete import failed.

MozReview-Commit-ID: L33bIxlkj08

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note that the test is a mochitest, not wpt, because of the above-mentioned "who knows what the spec says" issue.

It also doesn't really exhaustively test the various load-aggregation stuff we do, though I _think_ I got that part right.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Make sure to consistently fail a sheet load if any of its imports fail

This fixes a race where we would fail if and only if our last-to-complete import failed.

MozReview-Commit-ID: L33bIxlkj08

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8956199 [details] [diff] [review]
Make sure to consistently fail a sheet load if any of its imports fail

Review of attachment 8956199 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Loader.cpp
@@ +1818,5 @@
>  {
> +  LOG(("css::Loader::SheetComplete, status: 0x%" PRIx32, static_cast<uint32_t>(aStatus)));
> +
> +  // If aStatus is a failure we need to mark this data failed.  We also
> +  // need to mark any parent of a failing data as failed and any sibling

s/parent/ancestors/.

Also, this is only later siblings, right? Do we have an invariant that only the leftmost sibling can have children? If so, can we assert this somehow? Would be good to document that here in any case.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> s/parent/ancestors/.

OK.  I figured that was clear from the transitive nature of failure, but I can fix the comment.

> Also, this is only later siblings, right?

Yes, but this entrypoint is only called for the "earliest" sibling.

> Do we have an invariant that only the leftmost sibling can have children?

Yes, because that's the sibling that manages the actual load.  The other ones just represent things that need to get notified.

> Would be good to document that here in any case.

Can do.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So I get failures on try from browser/base/content/test/static/browser_parsable_css.js like so:

  Loading chrome://devtools/content/netmonitor/src/assets/styles/httpi.css?always-parse-css-0.9397438901543237 threw an error!

Debugging now.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d99da728f77
Make sure to consistently fail a sheet load if any of its imports fail.  r=bholley

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6d99da728f77

Comment 16

[Intermittent Failures Robot]

15 failures in 747 pushes (0.02 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 5
* autoland: 5
* mozilla-inbound: 2
* mozilla-central: 2
* mozilla-beta: 1

Platform breakdown:
* linux64: 8
* linux32-nightly: 2
* linux32: 2
* osx-10-10: 1
* linux64-qr: 1
* linux64-devedition: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1442126&startday=2018-03-05&endday=2018-03-11&tree=all