Closed Bug 1442126 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: bzbarsky)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

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?
Blocks: 1438974
Flags: needinfo?(bobbyholley)
Duplicate of this bug: 1442116
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.
Assignee: nobody → bobbyholley
Flags: needinfo?(bobbyholley)
Flags: needinfo?(bobbyholley)
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?
Flags: needinfo?(bobbyholley) → needinfo?(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.
Flags: needinfo?(bzbarsky)
Boris and I discussed, sounds like he's going to write the patch and I'll review.
Assignee: bobbyholley → bzbarsky
Flags: needinfo?(bzbarsky)
This fixes a race where we would fail if and only if our last-to-complete import failed.

MozReview-Commit-ID: L33bIxlkj08
Attachment #8956198 - Flags: review?(bobbyholley)
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.
Flags: needinfo?(bzbarsky)
This fixes a race where we would fail if and only if our last-to-complete import failed.

MozReview-Commit-ID: L33bIxlkj08
Attachment #8956199 - Flags: review?(bobbyholley)
Attachment #8956198 - Attachment is obsolete: true
Attachment #8956198 - Flags: review?(bobbyholley)
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.
Attachment #8956199 - Flags: review?(bobbyholley) → review+
> 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.
Blocks: 1443344
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.
Depends on: 1443393
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
https://hg.mozilla.org/mozilla-central/rev/6d99da728f77
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.