Closed Bug 1088183 Opened 10 years ago Closed 10 years ago

[e10s] make top window URI available in the parent http channel

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
e10s m4+ ---

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(1 file, 6 obsolete files)

Propagate the top window URI to the parent http channel through HttpChannelOpenArgs (
19.51 KB, patch
mmc
: review+
Details | Diff | Splinter Review 
From netwerk/base/src/nsChannelClassifier:

   nsCOMPtr<nsIDOMWindow> win;
    rv = thirdPartyUtil->GetTopWindowForChannel(aChannel, getter_AddRefs(win));
    NS_ENSURE_SUCCESS(rv, rv);

    nsCOMPtr<nsIURI> uri;
    rv = thirdPartyUtil->GetURIFromWindow(win, getter_AddRefs(uri));

This code is used to check the allowlist for tracking protection. It doesn't really need GetTopWindowForChannel, it just needs GetURIForTopWindow (which doesn't exist in the idl)
Assignee: nobody → mmc
tracking-e10s: ? → m4+
Not ready for review, waiting for bug 1049299 to land to avoid rebase race.
Attachment #8512821 - Attachment is obsolete: true
Summary: [e10s] mozThirdPartyUtil.GetTopWindowForChannel isn't available in the parent process → [e10s] make top window URI available in the parent process
Summary: [e10s] make top window URI available in the parent process → [e10s] make top window URI available in the parent http channel
Attachment #8512978 - Attachment is obsolete: true
Comment on attachment 8513113 [details] [diff] [review]
Propagate the top window URI to the parent http channel through HttpChannelOpenArgs

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

This change calculates the top window URI in HttpChannelChild and saves it in the to nsHttpChannel through HttpChannelOpenArgs.
Attachment #8513113 - Flags: review?(mrbkap)
Attachment #8513113 - Flags: review?(jduell.mcbugs)
Comment on attachment 8513113 [details] [diff] [review]
Propagate the top window URI to the parent http channel through HttpChannelOpenArgs

Hmm, I thought I fixed the serializing null principals bug after talking to Blake yesterday but it is still showing up in my try run.
Attachment #8513113 - Flags: review?(mrbkap)
Attachment #8513113 - Flags: review?(jduell.mcbugs)
Attachment #8513113 - Attachment is obsolete: true
Comment on attachment 8514003 [details] [diff] [review]
Propagate the top window URI to the parent http channel through HttpChannelOpenArgs

OK, try is looking better. The broken build in linux dbg is due to a missing include that only matters for non-unified build.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=45b9068c5a33
Attachment #8514003 - Flags: review?(mrbkap)
Attachment #8514003 - Flags: review?(jduell.mcbugs)
Comment on attachment 8514003 [details] [diff] [review]
Propagate the top window URI to the parent http channel through HttpChannelOpenArgs

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

::: netwerk/base/src/nsChannelClassifier.cpp
@@ +148,5 @@
>      // Tracking protection will be disabled so update the security state
>      // of the document and fire a secure change event.
> +    nsCOMPtr<nsIDOMWindow> win;
> +    rv = thirdPartyUtil->GetTopWindowForChannel(aChannel, getter_AddRefs(win));
> +    NS_ENSURE_SUCCESS(rv, rv);

So I'm confused about what happens here on the parent for an e10s channel.  Looks like you'll get a null TopWindow and return an error.  Is that what you want?  Maybe a comment would be nice.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1342,5 @@
> +    util = do_GetService(THIRDPARTYUTIL_CONTRACTID);
> +  }
> +  if (util) {
> +    // In e10s, this must be computed in the child. The parent gets the top
> +    // window URI through HttpChannelOpenArgs.

Move this comment up to the "if (!mTopWindowURI)" comment, so:

  // Only compute the top window URI once. (In e10s we compute it on the child, then set mTopWindowURI on the parent via HttpChannelOpenArgs)

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1498,5 @@
>      mThirdPartyFlags |= thirdParty ?
>        nsIHttpChannelInternal::THIRD_PARTY_PARENT_IS_THIRD_PARTY :
>        nsIHttpChannelInternal::THIRD_PARTY_PARENT_IS_SAME_PARTY;
> +    nsCOMPtr<nsIURI> uri;
> +    (void)GetTopWindowURI(getter_AddRefs(uri));

No need to put (void) in front of functions that ignore return value. That's ancient K&R C style formatting :)  That said, if you think it helps indicate expressly that we don't care about return value, I'm ok with leaving it.  But we don't use that idiom much in necko.
Attachment #8514003 - Flags: review?(jduell.mcbugs) → review+
Comment on attachment 8514003 [details] [diff] [review]
Propagate the top window URI to the parent http channel through HttpChannelOpenArgs

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

This looks good to me.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1332,5 @@
>  // HttpBaseChannel::nsIHttpChannelInternal
>  //-----------------------------------------------------------------------------
>  
>  NS_IMETHODIMP
> +HttpBaseChannel::GetTopWindowURI(nsIURI **aTopWindowURI)

I'm not thrilled with this, but I don't really see a better way of doing it.

@@ +1340,5 @@
> +  // Only compute the top window URI once.
> +  if (!mTopWindowURI) {
> +    util = do_GetService(THIRDPARTYUTIL_CONTRACTID);
> +  }
> +  if (util) {

I don't really see a reason to pull this out of the previous if statement.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1498,5 @@
>      mThirdPartyFlags |= thirdParty ?
>        nsIHttpChannelInternal::THIRD_PARTY_PARENT_IS_THIRD_PARTY :
>        nsIHttpChannelInternal::THIRD_PARTY_PARENT_IS_SAME_PARTY;
> +    nsCOMPtr<nsIURI> uri;
> +    (void)GetTopWindowURI(getter_AddRefs(uri));

It's a little cleaner to use |unused <<| instead of (void).

::: netwerk/protocol/http/nsHttpChannel.h
@@ +151,5 @@
>          return NS_OK;
>      }
>  
> +    nsresult SetTopWindowURI(nsIURI* aTopWindowURI) {
> +      mTopWindowURI = aTopWindowURI;

Nit: this file is inconsistent, but the modeline says to use 4-space tabs.
Attachment #8514003 - Flags: review?(mrbkap) → review+
Comment on attachment 8518329 [details] [diff] [review]
Propagate the top window URI to the parent http channel through HttpChannelOpenArgs (

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

Address comments, thanks for the reviews!
Attachment #8518329 - Flags: review+
Attachment #8518329 - Attachment is obsolete: true
Attachment #8514003 - Attachment is obsolete: true
Attachment #8518599 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f129b17f9067
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: