Open Bug 1113676 Opened 7 years ago Updated 1 year ago

img resources missing from resource-timing entries when loaded from cache


(Core :: DOM: Core & HTML, defect, P3)





(Reporter: valentin, Unassigned)


(Blocks 2 open bugs)


(Whiteboard: [webpagetest])


(2 files)

In imgLoader::LoadImage, when loading an image from cache, the imgRequestProxy is missing an mChannel/mTimedChannel and it does not go through call to nsHttpChannel::OnStopRequest so it could get added to the entry list.

Related to bug 1021221.
Depends on: 1113683
Comment on attachment 8542558 [details] [diff] [review]
img resources missing from resource-timing entries when loaded from cache

Review of attachment 8542558 [details] [diff] [review]:

Looks good. r+ with the issues below fixed.

Fundamentally the design of imgLoader is pretty bad, and this feels like yet another hack to me, but I don't see a way around it without a major rework of imgLoader. (Which will happen eventually.)

::: image/src/imgLoader.cpp
@@ +2159,5 @@
> +      // performance object. We need to add the entry here.
> +      if (doc && !request->mChannel) {
> +        nsPerformance* perf = doc->GetInnerWindow() ?
> +          doc->GetInnerWindow()->GetPerformance() : nullptr;
> +        nsCOMPtr<nsIHttpChannel> httpChan(do_QueryInterface(request->mTimedChannel));

Since you don't need this unless |perf| is non-null, move this variable inside the |if| below.

::: image/src/imgRequest.cpp
@@ +36,5 @@
>  #include "nsIProtocolHandler.h"
>  #include "imgIRequest.h"
> +#include "mozilla/net/NullHttpChannel.h"
> +using mozilla::net::NullHttpChannel;

Please group this |using| directive with the other |using| directives.

@@ +119,5 @@
>    mRequest = aRequest;
>    mChannel = aChannel;
> +  if (mChannel) {
> +    mTimedChannel = do_QueryInterface(mChannel);
> +  }

This is a more verbose way of saying the same thing as the original code. Please change it back. (I assume you want to preserve the dummy |mTimedChannel| created in OnStopRequest, but OnStopRequest should never be called before Init.)

@@ +1098,5 @@
>    mChannel = mNewRedirectChannel;
> +  if (mChannel) {
> +    mTimedChannel = do_QueryInterface(mChannel);
> +  }

This needs a comment explaining that you're trying to preserve the mTimedChannel you may have created in OnStopRequest. (That is what you're doing, right?) Otherwise someone in the future, doing some refactoring, is going to see this and assume that it does exactly the same thing as |mTimedChannel = do_QueryInterface(mChannel)| and change it back.
Attachment #8542558 - Flags: review?(seth) → review+
Comment on attachment 8542558 [details] [diff] [review]
img resources missing from resource-timing entries when loaded from cache

>+  if (mChannel) {
>+    mTimedChannel = do_QueryInterface(mChannel);

do_QI is null-safe.  Why is the null-check needed?  If you actually want to preserve the old mTimedChannel value here, please document that clearly so someone doesn't helpfully remove the "useless" null-check.  On the other hand, if the null-check is in fact useless, please remove it.

This pattern occurs in two places in this patch.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8542561 [details] [diff] [review]
Add test for cached resource timing entries

>+  dump("LOADED");

Let's not add a dependency on the non-standard dump() function.

>+  dynamicAddImage(startSecondTest);

Shouldn't the argument there be firstCheckAsync?

>+  gIframe = document.createElement("iframe");

Please add:

  gIframe.setAttribute("type", "content");

after that line.

>+// an nsIWebProgressListener that checks all requests made by the docShell

Why couldn't you just use a load event on the <iframe> element?  Please document.

Does this test fail without the imgLoader changes?

r=me if so with the above issues fixed.
Attachment #8542561 - Flags: review?(bzbarsky) → review+
What is the status of this bug and its patch?
Blocks: 1381443
What is the status of this bug?
There might still be an issue with the patch which prevented me from landing it before.
I pushed to try to see if changes in imgLoader might have changed that situation.
Flags: needinfo?(valentin.gosu)
Priority: -- → P3
Blocks: 1399535
Any change we can get this prioritized? This helps when we collect RUM data and want to know the full page size and comparing with transfered and cached assets.
Whiteboard: [webpagetest]
Component: DOM → DOM: Core & HTML
Duplicate of this bug: 1542396

If it' okay with everyone, I can take a whack at testing this patch. Unless I hear otherwise, I will add it to my TODO list.

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Not currently working on this.

Assignee: valentin.gosu → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.