1256595 - Make nsILoadInfo.(inner|outer)WindowId consistent across node types, i.e. use frame(Outer|Inner)WindowId for subdocuments.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Giorgio Maone [:ma1]]

From Bug  comment 10:

>> [:billm]
>> This patch changes the value of mInnerWindowID in some cases. 
>> When we're loading an <iframe src="f1163861oo.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.

> [:sicking]
> 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.

I'm gonna block this bug on another WebExtensions bug I'm about to file in order to keep webRequest's behavior resilient to this change.

Comment 1

[Kershaw Chang [:kershaw]]

Attachment: nsILoadInfo.idl changes

Hi Jonas,

Could you take a quick look at this IDL changes? I'd like to make sure this is the correct way to go.

Thanks.

Comment 2

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

I think this looks good for innerWindowID, frameInnerWindowId and frameOuterWindowId.

But I think we should still have a outerWindowID which matches the innerWindowId. I.e. it would return the outer window id of the loadingDocument, or in case of document loads, the outer window id of the targetted window.

If we do that, then I also don't think there's a need for parentFrameOuterWindowID.


One thing that I don't understand is how frameInnerWindowId is intended to work. When we are loading a <frame>, I thought we didn't create the inner window until after we start receiving data from the server. So how can frameInnerWindowId return anything if the window hasn't been created?
What does innerWindowID currently return when we are loading a <frame>?


On a related note, it would be really great to add something like `topFrameElement` or `topOuterWindowId`. That would help a lot in e10s. However I think that's probably a separate followup bug.

Comment 3

[Kershaw Chang [:kershaw]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> I think this looks good for innerWindowID, frameInnerWindowId and
> frameOuterWindowId.
> 
> But I think we should still have a outerWindowID which matches the
> innerWindowId. I.e. it would return the outer window id of the
> loadingDocument, or in case of document loads, the outer window id of the
> targetted window.
> 
> If we do that, then I also don't think there's a need for
> parentFrameOuterWindowID.
> 
> 
> One thing that I don't understand is how frameInnerWindowId is intended to
> work. When we are loading a <frame>, I thought we didn't create the inner
> window until after we start receiving data from the server. So how can
> frameInnerWindowId return anything if the window hasn't been created?
> What does innerWindowID currently return when we are loading a <frame>?
> 
When loading an iframe, the innerWindowID is 0 in current code [1].

> 
> On a related note, it would be really great to add something like
> `topFrameElement` or `topOuterWindowId`. That would help a lot in e10s.
> However I think that's probably a separate followup bug.

Will file another bug.

[1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/netwerk/base/LoadInfo.cpp#125

Comment 4

[Kershaw Chang [:kershaw]]

I've made two tables for describing what we are going to do.

Assume the current inner window ID is 2 and the outer window ID is 1.
There are two cases:
1. Load an image.
2. Load an iframe.

The result of the current LoadInfo is:
+--------------------+------------+-------------+
|                    | Load image | Load iframe |
+-----------------------------------------------+
|innerWindowID       |     2      |      0      |
+-----------------------------------------------+
|outerWindowID       |     1      |      3      |
+-----------------------------------------------+
|parentOuterWindowID |     1      |      1      |
+--------------------+------------+-------------+

We are going to modify LoadInfo and the new result will be:
+-------------------+------------+-------------+
|                   | Load image | Load iframe |
+----------------------------------------------+
|innerWindowID      |      2     |      2      |
+----------------------------------------------+
|outerWindowID      |      1     |      1      |
+----------------------------------------------+
|frameInnerWindowId |      0     |      0      |
+----------------------------------------------+
|frameOuterWindowId |      0     |      3      |
+-------------------+------------+-------------+

Not sure if I understand the code correctly.
@Jonas, could you correct me if I was wrong?
In addition, it looks like frameInnerWindowId should be always 0. Do we really need to add it?

Comment 5

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

> We are going to modify LoadInfo and the new result will be:
> +-------------------+------------+-------------+
> |                   | Load image | Load iframe |
> +----------------------------------------------+
> |innerWindowID      |      2     |      2      |
> +----------------------------------------------+
> |outerWindowID      |      1     |      1      |
> +----------------------------------------------+
> |frameInnerWindowId |      0     |      0      |
> +----------------------------------------------+
> |frameOuterWindowId |      0     |      3      |
> +-------------------+------------+-------------+

This looks great. I agree that we should remove frameInnerWindowId.

The only thing that I would add is that when we make this switch, we should look at existing uses of loadInfo.innerWindowID/outerWindowID and modify it so that it works with these new values.

Thanks so much for doing this!


Somewhat relatedly, it would be great if we could make these properties work for requests coming from workers. The best way to accomplish that would be to fix bug 1259873. But we should do that separately of course.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Add frameOuterWindowID in nsILoadInfo

I found that we still need to keep parentOuterWindowID in order to determine whether it is a top level window [1]. So, I just add frameOuterWindowID for subdocuments loading.
The window IDs can be summarized like:
+-------------------+------------+-------------+
|                   | Load image | Load iframe |
+----------------------------------------------+
|innerWindowID      |      2     |      2      |
+----------------------------------------------+
|outerWindowID      |      1     |      1      |
+----------------------------------------------+
|parentOuterWindowID|      1     |      1      |
+----------------------------------------------+
|frameOuterWindowID |      0     |      3      |
+-------------------+------------+-------------+


Hi Christoph,
Since Jonas is on PTO, could you take a look at this patch?
Thanks.

[1] http://searchfox.org/mozilla-central/rev/970569ad57ac4436ff31aa2ac63f38ed3ee2932d/devtools/shared/webconsole/network-helper.js#275

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8764529 [details] [diff] [review]
Add frameOuterWindowID in nsILoadInfo

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

The changes look good to me, but I would feel more comfortable if Bill could also sign off on it.

Comment 8

[Kershaw Chang [:kershaw]]

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

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Add frameOuterWindowID in nsILoadInfo, r=billm

Carry reviewer's name.

Thanks for reviewing.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6552880a34
Add frameOuterWindowID in nsILoadInfo, r=billm

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fa6552880a34