1107638 - Add innerWindowID to LoadInfo

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

In our meeting today on how to improve nsSecureBrowserUIImpl (see Bug 832834) we decided to add the innerWindowId to the LoadInfo [1] as a first step so we can handle false positives (incorrect appearances of the icon, eg. the grey triangle) that occur because nsSecureBrowserUIImpl attributes loads incorrectly. E.g window.onunload() loads an image which is then attributed incorrectly to the new page.

While using the LoadingNode from the LoadInfo would work in regular mode, we need something that also works in e10s, because the node is not serializeable in e10s. Hence we decided to use the innerWindowID for that purpose so we can uniquely identify the current window.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadInfo.h#23
uint64_t mInnerWindowID;

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: windowid_loadinfo.patch

Jonas, Jason, in this patch we are adding the innerWindowID to the LoadInfo. In the most common case we can query the innerWindowID from the node/document handed to the constructor.

Now, for e10s, we don't have a node available, hence I created a special private constructor and declared HttpChannelParent and FtpChannelParent as friend classes. Initially I wanted to add a friend method only, but ran into all these namespace problems because we would actually have to include httpChannelParent in LoadInfo instead of using a froward declaration. Anyway, I think the intention is clear. No one besides Http/FTPChannelParent should call that special constructor of LoadInfo.

Comment 2

[Jason Duell]

Comment on attachment 8542614 [details] [diff] [review]
windowid_loadinfo.patch

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

I'm not sure we need sicking's review here if he's busy with other things.  I'll leave that call to ckerschb

::: docshell/base/LoadInfo.h
@@ +49,5 @@
> +           nsContentPolicyType aContentPolicyType,
> +           uint32_t aInnerWindowID);
> +
> +  friend class net::HttpChannelParent;
> +  friend class net::FTPChannelParent;

I personally consider a lot of C++ protections to be overengineering, but if you want to keep the constructor private I'm fine with that.  I'd also be fine with it being public and adding a note on when it should be used.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +260,5 @@
> +                             loadInfo,
> +                             nullptr,   // loadGroup
> +                             nullptr,   // aCallbacks
> +                             loadFlags,
> +                             ios);

nit: I'm generally not a fan of the one-arg-per-line function call format--it takes up too much page space (can view less code in a window w/o scrolling).  We don't use it in necko very often.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #2)
> I'm not sure we need sicking's review here if he's busy with other things. 
> I'll leave that call to ckerschb

Thanks Jason for the speedy response, I think I would like Jonas to have a look at this patch as well, because we are performing changes to the loadInfo itself.
 
> I personally consider a lot of C++ protections to be overengineering, but if
> you want to keep the constructor private I'm fine with that.

Generally I agree, but on the other hand people tend to ignore a comment rather than a private constr which might turn into a compile error.

> nit: I'm generally not a fan of the one-arg-per-line function call
> format--it takes up too much page space (can view less code in a window w/o
> scrolling).  We don't use it in necko very often.

I know you don't like that - I'll fix that once I get Jonas' review.

Happy New Year!

Comment 4

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

Comment on attachment 8542614 [details] [diff] [review]
windowid_loadinfo.patch

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

Looks good!

I'm also fine with just using a comment rather than the 'friend' stuff. But I'm fine either way. We can always change it if we end up having to add too many 'friend's.

::: docshell/base/LoadInfo.cpp
@@ +28,5 @@
>    , mSecurityFlags(aSecurityFlags)
>    , mContentPolicyType(aContentPolicyType)
>    , mBaseURI(aBaseURI)
> +  , mInnerWindowID((aLoadingContext && aLoadingContext->GetOwnerDocument()) ?
> +                      aLoadingContext->GetOwnerDocument()->InnerWindowID() : 0)

Change GetOwnerDocument() to OwnerDoc(). GetOwnerDocument() returns null if you call it on a document, which is not what you want here. OwnerDoc() returns 'this' if called on a document.

Comment 5

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

Actually, when you change GetOwnerDocument() to OwnerDoc(), it also means that you can remove the second nullcheck since OwnerDoc() never returns null. You still need to nullcheck aLoadingContext of course.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: windowid_loadinfo_v2.patch

Thanks Jonas, Jason - I incorporated your nits and suggestions! Here is an updated version! Carrying over r+ from Jonas and Jason!

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eab4b3520c50

Comment 8

[Carsten Book [:Tomcat]]

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