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

NEW
Assigned to

Status

()

Core
DOM
P3
normal
3 years ago
2 months ago

People

(Reporter: valentin, Assigned: valentin)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 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

3 years ago
Depends on: 1113683
(Assignee)

Comment 1

3 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

3 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+
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee85b68e8d18
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=69c49e9b0357
(Assignee)

Comment 8

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=535412106630
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2ae06b28925
(Assignee)

Comment 10

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

Comment 11

5 months ago
What is the status of this bug and its patch?
(Assignee)

Updated

3 months ago
Blocks: 1381443
What is the status of this bug?
(Assignee)

Comment 13

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df64560f6a5c133ed204c579e1aa279f554029da
(Assignee)

Comment 14

2 months 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

2 months ago
Blocks: 1399535
You need to log in before you can comment on or make changes to this bug.