Bug 1442126. Make sure to consistently fail a sheet load if any of its imports fail. r=bholley

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

MozReview-Commit-ID: L33bIxlkj08

layout/style/Loader.cpp

@@ -170,6 +170,7 @@ SheetLoadData::SheetLoadData(Loader* aLoader,
   , mSheetAlreadyComplete(false)
   , mIsCrossOriginNoCORS(false)
   , mBlockResourceTiming(false)
+  , mLoadFailed(false)
   , mOwningElement(aOwningElement)
   , mObserver(aObserver)
   , mLoaderPrincipal(aLoaderPrincipal)
@@ -205,6 +206,7 @@ SheetLoadData::SheetLoadData(Loader* aLoader,
   , mSheetAlreadyComplete(false)
   , mIsCrossOriginNoCORS(false)
   , mBlockResourceTiming(false)
+  , mLoadFailed(false)
   , mOwningElement(nullptr)
   , mObserver(aObserver)
   , mLoaderPrincipal(aLoaderPrincipal)
@@ -250,6 +252,7 @@ SheetLoadData::SheetLoadData(Loader* aLoader,
   , mSheetAlreadyComplete(false)
   , mIsCrossOriginNoCORS(false)
   , mBlockResourceTiming(false)
+  , mLoadFailed(false)
   , mOwningElement(nullptr)
   , mObserver(aObserver)
   , mLoaderPrincipal(aLoaderPrincipal)
@@ -316,9 +319,9 @@ SheetLoadData::FireLoadEvent(nsIThreadInternal* aThread)
 
   nsContentUtils::DispatchTrustedEvent(node->OwnerDoc(),
                                        node,
-                                       NS_SUCCEEDED(mStatus) ?
-                                         NS_LITERAL_STRING("load") :
-                                         NS_LITERAL_STRING("error"),
+                                       mLoadFailed ?
+                                         NS_LITERAL_STRING("error") :
+                                         NS_LITERAL_STRING("load"),
                                        false, false);
 
   // And unblock onload
@@ -326,14 +329,12 @@ SheetLoadData::FireLoadEvent(nsIThreadInternal* aThread)
 }
 
 void
-SheetLoadData::ScheduleLoadEventIfNeeded(nsresult aStatus)
+SheetLoadData::ScheduleLoadEventIfNeeded()
 {
   if (!mOwningElement) {
     return;
   }
 
-  mStatus = aStatus;
-
   nsCOMPtr<nsIThread> thread = do_GetCurrentThread();
   nsCOMPtr<nsIThreadInternal> internalThread = do_QueryInterface(thread);
   if (NS_SUCCEEDED(internalThread->AddObserver(this))) {
@@ -1815,13 +1816,21 @@ Loader::DoParseSheetServo(ServoStyleSheet* aSheet,
 void
 Loader::SheetComplete(SheetLoadData* aLoadData, nsresult aStatus)
 {
-  LOG(("css::Loader::SheetComplete"));
+  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 ancestors of a failing data as failed and any sibling of a
+  // failing data as failed.  Note that SheetComplete is never called on a
+  // SheetLoadData that is the mNext of some other SheetLoadData.
+  if (NS_FAILED(aStatus)) {
+    MarkLoadTreeFailed(aLoadData);
+  }
 
   // 8 is probably big enough for all our common cases.  It's not likely that
   // imports will nest more than 8 deep, and multiple sheets with the same URI
   // are rare.
   AutoTArray<RefPtr<SheetLoadData>, 8> datasToNotify;
-  DoSheetComplete(aLoadData, aStatus, datasToNotify);
+  DoSheetComplete(aLoadData, datasToNotify);
 
   // Now it's safe to go ahead and notify observers
   uint32_t count = datasToNotify.Length();
@@ -1855,16 +1864,13 @@ Loader::SheetComplete(SheetLoadData* aLoadData, nsresult aStatus)
 }
 
 void
-Loader::DoSheetComplete(SheetLoadData* aLoadData, nsresult aStatus,
-                        LoadDataArray& aDatasToNotify)
+Loader::DoSheetComplete(SheetLoadData* aLoadData, LoadDataArray& aDatasToNotify)
 {
   LOG(("css::Loader::DoSheetComplete"));
   NS_PRECONDITION(aLoadData, "Must have a load data!");
   NS_PRECONDITION(aLoadData->mSheet, "Must have a sheet");
   NS_ASSERTION(mSheets, "mLoadingDatas should be initialized by now.");
 
-  LOG(("Load completed, status: 0x%" PRIx32, static_cast<uint32_t>(aStatus)));
-
   // Twiddle the hashtables
   if (aLoadData->mURI) {
     LOG_URI("  Finished loading: '%s'", aLoadData->mURI);
@@ -1897,7 +1903,7 @@ Loader::DoSheetComplete(SheetLoadData* aLoadData, nsresult aStatus,
       MOZ_ASSERT(!data->mSheet->HasForcedUniqueInner(),
                  "should not get a forced unique inner during parsing");
       data->mSheet->SetComplete();
-      data->ScheduleLoadEventIfNeeded(aStatus);
+      data->ScheduleLoadEventIfNeeded();
     }
     if (data->mMustNotify && (data->mObserver || !mObservers.IsEmpty())) {
       // Don't notify here so we don't trigger script.  Remember the
@@ -1920,17 +1926,17 @@ Loader::DoSheetComplete(SheetLoadData* aLoadData, nsresult aStatus,
     if (data->mParentData &&
         --(data->mParentData->mPendingChildren) == 0 &&
         !data->mParentData->mIsBeingParsed) {
-      DoSheetComplete(data->mParentData, aStatus, aDatasToNotify);
+      DoSheetComplete(data->mParentData, aDatasToNotify);
     }
 
     data = data->mNext;
   }
 
   // Now that it's marked complete, put the sheet in our cache.
-  // If we ever start doing this for failure aStatus, we'll need to
+  // If we ever start doing this for failed loads, we'll need to
   // adjust the PostLoadEvent code that thinks anything already
   // complete must have loaded succesfully.
-  if (NS_SUCCEEDED(aStatus) && aLoadData->mURI) {
+  if (!aLoadData->mLoadFailed && aLoadData->mURI) {
     // Pick our sheet to cache carefully.  Ideally, we want to cache
     // one of the sheets that will be kept alive by a document or
     // parent sheet anyway, so that if someone then accesses it via
@@ -1973,6 +1979,24 @@ Loader::DoSheetComplete(SheetLoadData* aLoadData, nsresult aStatus,
   NS_RELEASE(aLoadData);  // this will release parents and siblings and all that
 }
 
+void
+Loader::MarkLoadTreeFailed(SheetLoadData* aLoadData)
+{
+  if (aLoadData->mURI) {
+    LOG_URI("  Load failed: '%s'", aLoadData->mURI);
+  }
+
+  do {
+    aLoadData->mLoadFailed = true;
+
+    if (aLoadData->mParentData) {
+      MarkLoadTreeFailed(aLoadData->mParentData);
+    }
+
+    aLoadData = aLoadData->mNext;
+  } while (aLoadData);
+}
+
 nsresult
 Loader::LoadInlineStyle(nsIContent* aElement,
                         const nsAString& aBuffer,
@@ -2538,10 +2562,12 @@ Loader::PostLoadEvent(nsIURI* aURI,
     evt->mSheetAlreadyComplete = true;
 
     // If we get to this code, aSheet loaded correctly at some point, so
-    // we can just use NS_OK for the status.  Note that we do this here
-    // and not from inside our SheetComplete so that we don't end up
-    // running the load event async.
-    evt->ScheduleLoadEventIfNeeded(NS_OK);
+    // we can just schedule a load event and don't need to touch the
+    // data's mLoadFailed.  Note that we do this here and not from
+    // inside our SheetComplete so that we don't end up running the load
+    // event async.
+    MOZ_ASSERT(!evt->mLoadFailed, "Why are we marked as failed?");
+    evt->ScheduleLoadEventIfNeeded();
   }
 
   return rv;

layout/style/Loader.h

@@ -614,8 +614,12 @@ private:
   // The guts of SheetComplete.  This may be called recursively on parent datas
   // or datas that had glommed on to a single load.  The array is there so load
   // datas whose observers need to be notified can be added to it.
-  void DoSheetComplete(SheetLoadData* aLoadData, nsresult aStatus,
-                       LoadDataArray& aDatasToNotify);
+  void DoSheetComplete(SheetLoadData* aLoadData, LoadDataArray& aDatasToNotify);
+
+  // Mark the given SheetLoadData, as well as any of its siblings, parents, etc
+  // transitively, as failed.  The idea is to mark as failed any load that was
+  // directly or indirectly @importing the sheet this SheetLoadData represents.
+  void MarkLoadTreeFailed(SheetLoadData* aLoadData);
 
   StyleBackendType GetStyleBackendType() const;
 

layout/style/SheetLoadData.h

@@ -77,7 +77,7 @@ public:
 
   already_AddRefed<nsIURI> GetReferrerURI();
 
-  void ScheduleLoadEventIfNeeded(nsresult aStatus);
+  void ScheduleLoadEventIfNeeded();
 
   NotNull<const Encoding*> DetermineNonBOMEncoding(nsACString const& aSegment,
                                                    nsIChannel* aChannel);
@@ -177,6 +177,10 @@ public:
   // https://www.w3.org/TR/resource-timing/#processing-model
   bool mBlockResourceTiming : 1;
 
+  // Boolean flag indicating whether the load has failed.  This will be set
+  // to true if this load, or the load of any descendant import, fails.
+  bool mLoadFailed : 1;
+
   // This is the element that imported the sheet.  Needed to get the
   // charset set on it and to fire load/error events.
   nsCOMPtr<nsIStyleSheetLinkingElement> mOwningElement;
@@ -194,12 +198,6 @@ public:
   // is non-null.
   const Encoding* mPreloadEncoding;
 
-  // The status our load ended up with; this determines whether we
-  // should fire error events or load events.  This gets initialized
-  // by ScheduleLoadEventIfNeeded, and is only used after that has
-  // been called.
-  MOZ_INIT_OUTSIDE_CTOR nsresult mStatus;
-
 private:
   void FireLoadEvent(nsIThreadInternal* aThread);
 };

layout/style/test/mochitest.ini

@@ -252,6 +252,7 @@ skip-if = !stylo
 [test_keyframes_rules.html]
 [test_keyframes_vendor_prefix.html]
 [test_load_events_on_stylesheets.html]
+support-files = slow_broken_sheet.sjs slow_ok_sheet.sjs
 [test_logical_properties.html]
 [test_media_queries.html]
 skip-if = android_version == '18' #debug-only failure; timed out #Android 4.3 aws only; bug 1030419

layout/style/test/slow_broken_sheet.sjs

@@ -0,0 +1,16 @@
+// Make sure our timer stays alive.
+let gTimer;
+
+function handleRequest(request, response)
+{
+  response.setHeader("Content-Type", "text/html", false);
+  response.setStatusLine("1.1", 404, "Not Found");
+  response.processAsync();
+
+  gTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+  // Wait for 1s before responding; this should usually make sure this load comes in last.
+  gTimer.init(() => {
+    response.write("<h1>Hello</h1>");
+    response.finish();
+  }, 1000, Ci.nsITimer.TYPE_ONE_SHOT);
+}

layout/style/test/slow_ok_sheet.sjs

@@ -0,0 +1,18 @@
+// Make sure our timer stays alive.
+let gTimer;
+
+function handleRequest(request, response)
+{
+  response.setHeader("Content-Type", "text/css", false);
+  response.setStatusLine("1.1", 200, "OK");
+  response.processAsync();
+
+  gTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
+  // Wait for 1s before responding; this should usually make sure this load comes in last.
+  gTimer.init(() => {
+    // This sheet _does_ still get applied even though its importing link
+    // overall reports failure...
+    response.write("nosuchelement { background: red }");
+    response.finish();
+  }, 1000, Ci.nsITimer.TYPE_ONE_SHOT);
+}

layout/style/test/test_load_events_on_stylesheets.html

@@ -47,9 +47,9 @@ is(pendingEventCounter, 0, "There should be no pending events");
 // need to change that.
 window.onmessage = function() {
   messagePosted = true;
-  // There are 4 pending events: two from the two direct example.com loads,
-  // and 2 from the two data:text/css loads that import things
-  is(pendingEventCounter, 4, "Load event for sheet should have fired");
+  // There are 6 pending events: two from the two direct example.com loads,
+  // and 4 from the two data:text/css loads that import things
+  is(pendingEventCounter, 6, "Load event for sheet should have fired");
 }
 window.postMessage("", "*");
 
@@ -91,19 +91,19 @@ is(pendingEventCounter, 2, "There should be two pending events");
 ++pendingEventCounter;
 document.write('<link rel="stylesheet" href="http://www.example.com"\
                 onload="--pendingEventCounter;\
-                        ok(false, \'Load event firing on broken stylesheet\')"\
+                        ok(false, \'Load event firing on broken stylesheet 1\')"\
                 onerror="--pendingEventCounter;\
-                        ok(true, \'Error event firing on broken stylesheet\')">');
+                        ok(true, \'Error event firing on broken stylesheet 1\')">');
 
 ++pendingEventCounter;
 var link = document.createElement("link");
 link.rel = "stylesheet";
 link.href = "http://www.example.com";
 link.onload = function() { --pendingEventCounter;
-  ok(false, 'Load event firing on broken stylesheet');
+  ok(false, 'Load event firing on broken stylesheet 2');
 };
 link.onerror = function() { --pendingEventCounter;
-  ok(true, 'Error event firing on broken stylesheet');
+  ok(true, 'Error event firing on broken stylesheet 2');
 }
 document.body.appendChild(link);
 
@@ -144,15 +144,43 @@ link = document.createElement("link");
 link.rel = "stylesheet";
 link.href = "data:text/css,@import url('http://www.example.com')";
 link.onload = function() { --pendingEventCounter;
-  ok(false, 'Load event firing on broken stylesheet');
+  ok(false, 'Load event firing on broken stylesheet 3');
 };
 link.onerror = function() { --pendingEventCounter;
-  ok(true, 'Error event firing on broken stylesheet');
+  ok(true, 'Error event firing on broken stylesheet 3');
+}
+document.body.appendChild(link);
+
+function absoluteURL(relativeURL) {
+  return new URL(relativeURL, location.href).href;
+}
+
+++pendingEventCounter;
+link = document.createElement("link");
+link.rel = "stylesheet";
+link.href = `data:text/css,@import url('http://www.example.com'); @import url(${absoluteURL('slow_ok_sheet.sjs')});`;
+link.onload = function() { --pendingEventCounter;
+  ok(false, 'Load event firing on broken stylesheet 4');
+};
+link.onerror = function() { --pendingEventCounter;
+  ok(true, 'Error event firing on broken stylesheet 4');
+}
+document.body.appendChild(link);
+
+++pendingEventCounter;
+link = document.createElement("link");
+link.rel = "stylesheet";
+link.href = `data:text/css,@import url(${absoluteURL('slow_broken_sheet.sjs')}); @import url('data:text/css,');`;
+link.onload = function() { --pendingEventCounter;
+  ok(false, 'Load event firing on broken stylesheet 5');
+};
+link.onerror = function() { --pendingEventCounter;
+  ok(true, 'Error event firing on broken stylesheet 5');
 }
 document.body.appendChild(link);
 
 // Make sure the load events for all those stylesheets have not fired yet
-is(pendingEventCounter, 7, "There should be one pending event");
+is(pendingEventCounter, 9, "There should be nine pending events");
 
 </script>
 </pre>