1163861 - Include outer window ID and parent outer window ID in nsILoadInfo

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'd like to be able to use these in bug 1157561.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: webreq-patch

This patch changes WebRequest.jsm to expose the window IDs.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8614926 [details] [diff] [review]
patch

Review of attachment 8614926 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, assuming mInnerWindowID is still correct. I am not a netwerk peer though. I just chatted with Steve, he will sign off on it too.

::: netwerk/base/LoadInfo.cpp
@@ +66,5 @@
> +    }
> +
> +    if (outerWindow) {
> +      nsCOMPtr<nsPIDOMWindow> inner = outerWindow->GetCurrentInnerWindow();
> +      mInnerWindowID = inner ? inner->WindowID() : 0;

That for sure returns the same as
> aLoadingContext->OwnerDoc()->InnerWindowID()

If you say so, I trust you, I just don't want to end up in a situation where the mInnerWindowID is incorrectly not set in the loadInfo anymore.

::: netwerk/base/LoadInfo.h
@@ +68,5 @@
>    nsWeakPtr mLoadingContext;
>    nsSecurityFlags mSecurityFlags;
>    nsContentPolicyType mContentPolicyType;
>    nsCOMPtr<nsIURI> mBaseURI;
> +  uint64_t mInnerWindowID;

Thanks for fixing.

::: netwerk/ipc/NeckoChannelParams.ipdlh
@@ +64,5 @@
>    PrincipalInfo               requestingPrincipalInfo;
>    PrincipalInfo               triggeringPrincipalInfo;
>    uint32_t                    securityFlags;
>    uint32_t                    contentPolicyType;
> +  uint64_t                    innerWindowID;

Again, thanks for fixing.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Comment on attachment 8614926 [details] [diff] [review]
> patch
> 
> Review of attachment 8614926 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, assuming mInnerWindowID is still correct. I am not a netwerk peer
> though. I just chatted with Steve, he will sign off on it too.

Sorry, I should have called that out. This patch changes the value of mInnerWindowID in some cases. When we're loading an <iframe src="foo.html">, mInnerWindowID will now be the window ID of the foo.html document's window. Previously it was the window ID of the window containing the iframe. I made this change because it seems more useful and it's what Chrome does.

I guess I assumed that mInnerWindowID wasn't really used at the moment. If it is being used, I'll have to do something to separate the different meanings. Do you know if anything is using it?

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

I'm fine delegating to Chris here - he knows much more about LoadInfo than I do :) Please use his r+.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

> When we're loading an <iframe src="foo.html">,
> mInnerWindowID will now be the window ID of the foo.html document's window.
> Previously it was the window ID of the window containing the iframe. I made
> this change because it seems more useful and it's what Chrome does.
> 
> I guess I assumed that mInnerWindowID wasn't really used at the moment. If
> it is being used, I'll have to do something to separate the different
> meanings. Do you know if anything is using it?

Initially I introduced the windowId in Bug 1107638 because (back then) we thought using the windowId might help to get Bug 832834 [nsSecureBrowserUIImpl makes babies and kittens cry] rolling. Apparently this never happened, so I am pretty sure the windowId is still unused at the moment.

To give you some background, there is a problem (kind of a race condition) which gets triggered whenever you switch quite fast between two pages (e.g. by clicking the back and forward button over and over). In such a case the browserUI is not able to distinguish between requests still belonging to the previous page and requests belonging to the new page and hence the UI gets confused and displays for example a mixed content warning even though there is no mixed content on the page. Hence we decided to add the windowId of the toplevel page to the loadInfo so we can attribute requests to the correct page. Apparently this never happenend.

At the moment I am fine either way, but please add a few lines of documentation to the nsiLoadInfo.idl. Basically exactly what you just mentioned in Comment 4. So in case someone is going to use the innerWindowId, the person knows what to expect - thanks.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8614926 [details] [diff] [review]
patch

Clearing Steve's review request since he is fine with delegating the review to me.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc408deebe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/dace68100bfd

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cc408deebe7
https://hg.mozilla.org/mozilla-central/rev/dace68100bfd

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Bill McCloskey (:billm) from comment #4)
> Sorry, I should have called that out. This patch changes the value of
> mInnerWindowID in some cases. When we're loading an <iframe src="foo.html">,
> mInnerWindowID will now be the window ID of the foo.html document's window.
> Previously it was the window ID of the window containing the iframe. I made
> this change because it seems more useful and it's what Chrome does.

I think we should keep out internal APIs consistent. We shouldn't let loadInfo.innerWindowId refer to one thing for an <img> and another thing for an <iframe>.

I agree that the "inner" inner-window-id contains more information, since you can get the "outer" inner-window-id from the "inner" one. But I think consistency trumps maximizing information.

For example, the current setup makes it hard to block all loads from a given document.

I think what we should do is add another member called 'frameOuterWindowId' (and 'frameInnerWindowId') and let that value reflect the foo.html document's window ID in your example. And let it be 0 when loading <img>s.


What we reflect in the APIs that we expose to WebExtensions is IMO an orthogonal question. I completely defer to you (Bill) on that. Compatibility with Chrome sounds reasonable to me.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Bill, what do you think? (See comment 10 from Jonas).

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jonas Sicking (:sicking) from comment #10)
> I think we should keep out internal APIs consistent. We shouldn't let
> loadInfo.innerWindowId refer to one thing for an <img> and another thing for
> an <iframe>.
> 
> I agree that the "inner" inner-window-id contains more information, since
> you can get the "outer" inner-window-id from the "inner" one. But I think
> consistency trumps maximizing information.
> 
> For example, the current setup makes it hard to block all loads from a given
> document.
> 
> I think what we should do is add another member called 'frameOuterWindowId'
> (and 'frameInnerWindowId') and let that value reflect the foo.html
> document's window ID in your example. And let it be 0 when loading <img>s.

OK, that's fine with me. Christoph, would you be able to work on this? If not, I think Giorgio Maone could probably do it. He's been doing a lot of work on this stuff from the WebExtensions end.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Bill McCloskey (:billm) from comment #12)
> OK, that's fine with me. Christoph, would you be able to work on this? If
> not, I think Giorgio Maone could probably do it. He's been doing a lot of
> work on this stuff from the WebExtensions end.

If Giorgio can take it, that would be awesome. I have quite a backlog at the moment. Giorgio, any chance you can fix that up? If not, please let me know and we figure a way to move forward here.

Comment 14

[Giorgio Maone [:ma1]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> (In reply to Bill McCloskey (:billm) from comment #12)
> > OK, that's fine with me. Christoph, would you be able to work on this? If
> > not, I think Giorgio Maone could probably do it. He's been doing a lot of
> > work on this stuff from the WebExtensions end.
> 
> If Giorgio can take it, that would be awesome. I have quite a backlog at the
> moment. Giorgio, any chance you can fix that up? If not, please let me know
> and we figure a way to move forward here.

OK, I'm gonna clone this bug into a spin-off based on commen #10 and work from there.

Comment 15

[The 8472]

since webrequest doesn't expose the inner window ID, what's the correct way to associate requests with the frame's URL? since that could change between each request.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to The 8472 from comment #15)
> since webrequest doesn't expose the inner window ID, what's the correct way
> to associate requests with the frame's URL? since that could change between
> each request.

Is the frameId not what you need?