Bug 1076583 - Make src attribute not load sync. r=smaug,extension-rev…

…iewers,devtools-reviewers,robwu,nchevobbe

This fixes src loads to be consistent with srcset/picture loads, modulo
the special synchronous case in the spec
(https://html.spec.whatwg.org/#update-the-image-data step 7), which
requires src loads to be sync if the image is available.

We now avoid triggering the load from the parser consistently for src /
srcset / picture, and unify the codepath with BindToTree. That should
avoid some useless task allocations.

Only the sync load code-path needs a script runner (mostly to deal with
anonymous content like the video poster <img> and such, but it also
helps not trigger sync loads at unexpected times like on adoption).

About the HTMLImageElement::Complete() getter change, we need to also
return false if there's an existing load task. That is the proposal in
whatwg/html#4884, and prevents some failures
in the-img-element/{update-src-complete,img.complete}.html WPTs. It
technically changes our behavior on .srcset changes, but it makes it
consistent with .src changes and other browsers, so seems fine.

There are a couple regressions in CSP tests and the networkEvent stubs,
but these are really a pre-existing issue. What happens is that, since
the loads are now async, CSP can't figure out the script that triggered
the load anymore. I need to look if there's an easy way to propagate
that information in the image load tasks, but this is trivially
reproducible by changing these tests to use srcset rather than src.

The rest of the test changes are as expected: either new passes, or
expected test changes from this.

Differential Revision: https://phabricator.services.mozilla.com/D215519

devtools/client/webconsole/test/node/fixtures/stubs/networkEvent.js

@@ -29,14 +29,7 @@ rawPackets.set(`GET request`, {
   "cause": {
     "loadingDocumentUri": "https://example.com/browser/devtools/client/webconsole/test/browser/stub-generators/test-network-event.html",
     "type": "img",
-    "stacktraceAvailable": true,
-    "lastFrame": {
-      "filename": "https://example.com/browser/devtools/client/webconsole/test/browser/stub-generators/test-network-event.html",
-      "lineNumber": 3,
-      "columnNumber": 1,
-      "functionName": "triggerPacket",
-      "asyncCause": null
-    }
+    "stacktraceAvailable": false
   },
   "httpVersion": "HTTP/1.1",
   "status": "404",
@@ -68,14 +61,7 @@ rawPackets.set(`GET request update`, {
   "cause": {
     "loadingDocumentUri": "https://example.com/browser/devtools/client/webconsole/test/browser/stub-generators/test-network-event.html",
     "type": "img",
-    "stacktraceAvailable": true,
-    "lastFrame": {
-      "filename": "https://example.com/browser/devtools/client/webconsole/test/browser/stub-generators/test-network-event.html",
-      "lineNumber": 3,
-      "columnNumber": 1,
-      "functionName": "triggerPacket",
-      "asyncCause": null
-    }
+    "stacktraceAvailable": false
   },
   "httpVersion": "HTTP/1.1",
   "status": "404",

devtools/shared/commands/resource/tests/browser_resources_network_events_parent_process.js

@@ -21,8 +21,8 @@ const createParentProcessRequests = async () => {
 const EXPECTED_METHOD_NAME = "createParentProcessRequests";
 const EXPECTED_REQUEST_LINE_1 = 12;
 const EXPECTED_REQUEST_COL_1 = 9;
-const EXPECTED_REQUEST_LINE_2 = 17;
-const EXPECTED_REQUEST_COL_2 = 3;
+// const EXPECTED_REQUEST_LINE_2 = 17;
+// const EXPECTED_REQUEST_COL_2 = 3;
 
 // Test the ResourceCommand API around NETWORK_EVENT for the parent process
 
@@ -133,13 +133,20 @@ add_task(async function testParentProcessRequests() {
   ok(!firstImageRequest.fromCache, "The first image request isn't cached");
   ok(firstImageRequest.chromeContext, "The first image request is privileged");
 
-  const firstImageStacktrace = receivedStacktraces[1].lastFrame;
   is(receivedStacktraces[1].resourceId, firstImageRequest.stacktraceResourceId);
+  const firstImageStacktrace = receivedStacktraces[1].lastFrame;
+  // TODO(bug 1911435).
+  todo(
+    !!firstImageStacktrace,
+    "After bug 1076583, image load is async and we can't get a stack trace"
+  );
+  /*
   is(firstImageStacktrace.filename, gTestPath);
   is(firstImageStacktrace.lineNumber, EXPECTED_REQUEST_LINE_2);
   is(firstImageStacktrace.columnNumber, EXPECTED_REQUEST_COL_2);
   is(firstImageStacktrace.functionName, EXPECTED_METHOD_NAME);
   is(firstImageStacktrace.asyncCause, null);
+  */
 
   info("Assert the second image request");
   const secondImageRequest = receivedNetworkEvents[2];

dom/base/DOMIntersectionObserver.cpp

@@ -149,7 +149,7 @@ static void LazyLoadCallback(
     Element* target = entry->Target();
     if (entry->IsIntersecting()) {
       if (auto* image = HTMLImageElement::FromNode(target)) {
-        image->StopLazyLoading(HTMLImageElement::StartLoading::Yes);
+        image->StopLazyLoading();
       } else if (auto* iframe = HTMLIFrameElement::FromNode(target)) {
         iframe->StopLazyLoading();
       } else {

dom/base/nsImageLoadingContent.cpp

@@ -105,7 +105,6 @@ nsImageLoadingContent::nsImageLoadingContent()
       mRequestGeneration(0),
       mLoadingEnabled(true),
       mLoading(false),
-      mNewRequestsWillNeedAnimationReset(false),
       mUseUrgentStartForChannel(false),
       mLazyLoading(false),
       mStateChangerDepth(0),
@@ -971,7 +970,6 @@ nsImageLoadingContent::LoadImageWithChannel(nsIChannel* aChannel,
   if (NS_SUCCEEDED(rv)) {
     CloneScriptedRequests(req);
     TrackImage(req);
-    ResetAnimationIfNeeded();
     return NS_OK;
   }
 
@@ -1167,7 +1165,6 @@ nsresult nsImageLoadingContent::LoadImage(nsIURI* aNewURI, bool aForce,
 
     CloneScriptedRequests(req);
     TrackImage(req);
-    ResetAnimationIfNeeded();
 
     // Handle cases when we just ended up with a request but it's already done.
     // In that situation we have to synchronously switch that request to being
@@ -1483,10 +1480,6 @@ RefPtr<imgRequestProxy>& nsImageLoadingContent::PrepareCurrentRequest(
   // Get rid of anything that was there previously.
   ClearCurrentRequest(NS_BINDING_ABORTED, Some(OnNonvisible::DiscardImages));
 
-  if (mNewRequestsWillNeedAnimationReset) {
-    mCurrentRequestFlags |= REQUEST_NEEDS_ANIMATION_RESET;
-  }
-
   if (aImageLoadType == eImageLoadType_Imageset) {
     mCurrentRequestFlags |= REQUEST_IS_IMAGESET;
   }
@@ -1500,10 +1493,6 @@ RefPtr<imgRequestProxy>& nsImageLoadingContent::PreparePendingRequest(
   // Get rid of anything that was there previously.
   ClearPendingRequest(NS_BINDING_ABORTED, Some(OnNonvisible::DiscardImages));
 
-  if (mNewRequestsWillNeedAnimationReset) {
-    mPendingRequestFlags |= REQUEST_NEEDS_ANIMATION_RESET;
-  }
-
   if (aImageLoadType == eImageLoadType_Imageset) {
     mPendingRequestFlags |= REQUEST_IS_IMAGESET;
   }
@@ -1562,7 +1551,6 @@ void nsImageLoadingContent::MakePendingRequestCurrent() {
   mPendingRequestFlags = 0;
   mCurrentRequestRegistered = mPendingRequestRegistered;
   mPendingRequestRegistered = false;
-  ResetAnimationIfNeeded();
 }
 
 void nsImageLoadingContent::ClearCurrentRequest(
@@ -1606,16 +1594,6 @@ void nsImageLoadingContent::ClearPendingRequest(
   mPendingRequestFlags = 0;
 }
 
-void nsImageLoadingContent::ResetAnimationIfNeeded() {
-  if (mCurrentRequest &&
-      (mCurrentRequestFlags & REQUEST_NEEDS_ANIMATION_RESET)) {
-    nsCOMPtr<imgIContainer> container;
-    mCurrentRequest->GetImage(getter_AddRefs(container));
-    if (container) container->ResetAnimation();
-    mCurrentRequestFlags &= ~REQUEST_NEEDS_ANIMATION_RESET;
-  }
-}
-
 bool nsImageLoadingContent::HaveSize(imgIRequest* aImage) {
   // Handle the null case
   if (!aImage) return false;

dom/base/nsImageLoadingContent.h

@@ -424,13 +424,6 @@ class nsImageLoadingContent : public nsIImageLoadingContent {
       nsresult aReason,
       const Maybe<OnNonvisible>& aNonvisibleAction = Nothing());
 
-  /**
-   * Reset animation of the current request if
-   * |mNewRequestsWillNeedAnimationReset| was true when the request was
-   * prepared.
-   */
-  void ResetAnimationIfNeeded();
-
   /**
    * Static helper method to tell us if we have the size of a request. The
    * image may be null.
@@ -467,13 +460,11 @@ class nsImageLoadingContent : public nsIImageLoadingContent {
   uint8_t mPendingRequestFlags = 0;
 
   enum {
-    // Set if the request needs ResetAnimation called on it.
-    REQUEST_NEEDS_ANIMATION_RESET = 1 << 0,
     // Set if the request is currently tracked with the document.
-    REQUEST_IS_TRACKED = 1 << 1,
+    REQUEST_IS_TRACKED = 1 << 0,
     // Set if this is an imageset request, such as from <img srcset> or
     // <picture>
-    REQUEST_IS_IMAGESET = 1 << 2,
+    REQUEST_IS_IMAGESET = 1 << 1,
   };
 
   // If the image was blocked or if there was an error loading, it's nice to
@@ -565,16 +556,6 @@ class nsImageLoadingContent : public nsIImageLoadingContent {
    */
   bool mLoading : 1;
 
-  /**
-   * A hack to get animations to reset, see bug 594771. On requests
-   * that originate from setting .src, we mark them for needing their animation
-   * reset when they are ready. mNewRequestsWillNeedAnimationReset is set to
-   * true while preparing such requests (as a hack around needing to change an
-   * interface), and the other two booleans store which of the current
-   * and pending requests are of the sort that need their animation restarted.
-   */
-  bool mNewRequestsWillNeedAnimationReset : 1;
-
   /**
    * Flag to indicate whether the channel should be mark as urgent-start.
    * It should be set in *Element and passed to nsContentUtils::LoadImage.

dom/base/test/browser_blocking_image.js

@@ -158,8 +158,6 @@ add_task(async function block_pending_request_test() {
 
     let img = content.document.createElement("img");
     img.src = "https://example.com/tests/image/test/mochitest/shaver.png";
-
-    let req = img.getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST);
     img.addObserver(observer);
 
     content.document.body.appendChild(img);
@@ -169,6 +167,7 @@ add_task(async function block_pending_request_test() {
     info("Size Available now!");
     img.removeObserver(observer);
 
+    let req = img.getRequest(Ci.nsIImageLoadingContent.CURRENT_REQUEST);
     // Now we change to load from http:// site, which will be blocked.
     img.src = "http://example.com/tests/image/test/mochitest/shaver.png";