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

Status: NEW

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

Description

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

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

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Add test for cached resource timing entries

Comment 3

[Seth Fowler [:seth] [:s2h]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee85b68e8d18

Comment 7

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69c49e9b0357

Comment 8

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=535412106630

Comment 9

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2ae06b28925

Comment 10

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f46c5e1fbef1

Comment 11

[Timo Tijhof]

What is the status of this bug and its patch?

Comment 12

[Shubham Jindal [:shubham]]

What is the status of this bug?

Comment 13

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df64560f6a5c133ed204c579e1aa279f554029da

Comment 14

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 15

[Peter Hedenskog]

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.

Comment 17

[Will Hawkins]

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.

Comment 18

[Stephen Donner [:stephend] Not actively reading bugmail]

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

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Not currently working on this.

Comment 20

[Marcel Duran]

Bumping this bug in the hope of getting it prioritized.
Any updates?

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Blocks a p2 -> p2. We have patches, and this may fix bug 1852782