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

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?

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.

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?

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.

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

This fixes a race where we would fail if and only if our last-to-complete import failed. MozReview-Commit-ID: L33bIxlkj08

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.

This fixes a race where we would fail if and only if our last-to-complete import failed. MozReview-Commit-ID: L33bIxlkj08

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.

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

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.

Pushed by bzbarsky@mozilla.com: 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