Closed
Bug 1163861
Opened 9 years ago
Closed 9 years ago
Include outer window ID and parent outer window ID in nsILoadInfo
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
24.50 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
I'd like to be able to use these in bug 1157561.
Assignee | ||
Updated•9 years ago
|
Blocks: webextensions-chrome-gaps
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
This patch changes WebRequest.jsm to expose the window IDs.
Attachment #8614927 -
Flags: review?(dtownsend)
Comment 3•9 years ago
|
||
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.
Attachment #8614926 -
Flags: review?(sworkman)
Attachment #8614926 -
Flags: review?(mozilla)
Attachment #8614926 -
Flags: review+
Assignee | ||
Comment 4•9 years ago
|
||
(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?
Flags: needinfo?(mozilla)
Comment 5•9 years ago
|
||
I'm fine delegating to Chris here - he knows much more about LoadInfo than I do :) Please use his r+.
Comment 6•9 years ago
|
||
> 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.
Flags: needinfo?(mozilla)
Comment 7•9 years ago
|
||
Comment on attachment 8614926 [details] [diff] [review] patch Clearing Steve's review request since he is fine with delegating the review to me.
Attachment #8614926 -
Flags: review?(sworkman)
Updated•9 years ago
|
Attachment #8614927 -
Flags: review?(dtownsend) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cc408deebe7 https://hg.mozilla.org/integration/mozilla-inbound/rev/dace68100bfd
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6cc408deebe7 https://hg.mozilla.org/mozilla-central/rev/dace68100bfd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(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•8 years ago
|
||
Bill, what do you think? (See comment 10 from Jonas).
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 12•8 years ago
|
||
(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.
Flags: needinfo?(wmccloskey) → needinfo?(mozilla)
Comment 13•8 years ago
|
||
(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.
Flags: needinfo?(mozilla) → needinfo?(g.maone)
Comment 14•8 years ago
|
||
(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.
Flags: needinfo?(g.maone)
Comment 15•8 years ago
|
||
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.
Assignee | ||
Comment 16•8 years ago
|
||
(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?
You need to log in
before you can comment on or make changes to this bug.
Description
•