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

RESOLVED FIXED in Firefox 50

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: mao, Assigned: kershaw)

Tracking

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 affected, firefox50 fixed)

Details

(Whiteboard: [webRequest][necko-active])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
Depends on: 1256600
Whiteboard: [webRequest] → [webRequest][necko-backlog]
(Assignee)

Comment 1

2 years ago
Created attachment 8755692 [details] [diff] [review]
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.
Assignee: nobody → kechang
Attachment #8755692 - Flags: feedback?(jonas)
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.
Attachment #8755692 - Flags: feedback?(jonas)
(Assignee)

Comment 3

2 years ago
(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
Whiteboard: [webRequest][necko-backlog] → [webRequest][necko-active]
(Assignee)

Comment 4

2 years ago
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?
Flags: needinfo?(jonas)
> 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.
Flags: needinfo?(jonas)
(Assignee)

Comment 6

2 years ago
Created attachment 8764529 [details] [diff] [review]
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
Attachment #8755692 - Attachment is obsolete: true
Attachment #8764529 - Flags: review?(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.
Attachment #8764529 - Flags: review?(wmccloskey)
Attachment #8764529 - Flags: review?(ckerschb)
Attachment #8764529 - Flags: feedback+
Attachment #8764529 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 8

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

Comment 9

2 years ago
Created attachment 8765391 [details] [diff] [review]
Add frameOuterWindowID in nsILoadInfo, r=billm

Carry reviewer's name.

Thanks for reviewing.
Attachment #8764529 - Attachment is obsolete: true
Attachment #8765391 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6552880a34
Add frameOuterWindowID in nsILoadInfo, r=billm
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fa6552880a34
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Updated

a year ago
Depends on: 1295309
You need to log in before you can comment on or make changes to this bug.