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

NEW
Assigned to

Status

()

P3
normal
4 years ago
5 months ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Depends on: 1113683
(Assignee)

Comment 1

4 years ago
Created attachment 8542558 [details] [diff] [review]
img resources missing from resource-timing entries when loaded from cache
Attachment #8542558 - Flags: review?(seth)
(Assignee)

Comment 2

4 years ago
Created attachment 8542561 [details] [diff] [review]
Add test for cached resource timing entries
Attachment #8542561 - Flags: review?(bzbarsky)
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+

Comment 11

a year ago
What is the status of this bug and its patch?
(Assignee)

Updated

a year ago
Blocks: 1381443
What is the status of this bug?
(Assignee)

Comment 14

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1399535

Comment 15

5 months ago
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.
You need to log in before you can comment on or make changes to this bug.