mozbrowser doesn't send auth request off a redirect

VERIFIED FIXED in Firefox 18



7 years ago
4 months ago


(Reporter: cjones, Assigned: jduell.mcbugs)


(Blocks 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 verified)



(2 attachments, 2 obsolete attachments)

 (1) Load

(This redirects to a site protected by basic auth.)  There's no auth prompt, just a 401 error page.

If you navigate directly to the URL, the auth prompt is shown.
Sorry, pasted the wrong link: that should be

 (1) Load
Patrick, since you fixed the HTTP authentication crash in bug 775464, can you take a look here?
Assignee: nobody → pwang
Looks like that when Http request is redirected, the LoadContext of original HttpChannelParent isn't copied to the new HttpChannelParent created for handling redirected request.

In normal request, the LoadContext is passed through IPC and is handled by HttpChannelParent::RecvAsyncOpen() when a channel is opened. However, redirecting of a request doesn't call RecvAsyncOpen() of new HttpChannelParent. I think we would need a way to copy the LoadContext to new HttpChannelParent.
Hi Jason,

This patch makes HttpChannelChild send its LoadContext when HttpChannelChild::ConnectParent() is called, and HttpChannelParent can get the LoadContext after redirect. Would you help to give feedback on this patch?

Attachment #666936 - Flags: feedback?(jduell.mcbugs)
Comment on attachment 666936 [details] [diff] [review]
Send HttpChannelParent LoadContext when the request is redirected

Review of attachment 666936 [details] [diff] [review]:

You've got the diagnosis of the problem right--we need to make sure the redirected channel has a Loadcontext.

I'd rather fix this by making the PHttpChannel constructor take a SerializedLoadContext.  That needs to happen for some security work anyway, it'll make sure that we don't ever miss it, and it'll also be a little cleaner, I think.
Attachment #666936 - Flags: feedback?(jduell.mcbugs)
Like so...   I've verified that this gets us the auth prompt. 

(Sorry to steal bug Patrick--I already had some of this code written.)

jdm: if you have any doubts about reviewing this pass over to :mayhemer.
Assignee: pwang → jduell.mcbugs
Attachment #666936 - Attachment is obsolete: true
Attachment #668672 - Flags: review?(josh)
Comment on attachment 668672 [details] [diff] [review]
v2: fix by requiring loadContent in IPDL constructor

Review of attachment 668672 [details] [diff] [review]:

::: netwerk/ipc/PNecko.ipdl
@@ -42,5 @@
>    HTMLDNSPrefetch(nsString hostname, uint16_t flags);
>    CancelHTMLDNSPrefetch(nsString hostname, uint16_t flags, nsresult reason);
> -both:
> -  PHttpChannel(nullable PBrowser browser);

We now only receive this on the parent now, so we should have changed the IPDL file a while ago.  Grep -r and g++ appear to agree with me.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +807,5 @@
>    // until OnStopRequest, or we do a redirect, or we hit an IPDL error.
>    AddIPDLReference();
> +  if (!gNeckoChild->SendPHttpChannelConstructor(
> +                      this, tabChild, IPC::SerializedLoadContext(this))) {

This is the only real semantic change in the patch--everything else is boilerplate.
Comment on attachment 668672 [details] [diff] [review]
v2: fix by requiring loadContent in IPDL constructor

Review of attachment 668672 [details] [diff] [review]:

r- for the non-existent channel problem. I'd like to see the solution before giving this my blessing.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +56,5 @@
> +    } else {
> +      mLoadContext = new LoadContext(loadContext);
> +    }
> +  } else if (loadContext.IsPrivateBitValid()) {
> +    nsCOMPtr<nsIPrivateBrowsingChannel> pbChannel = do_QueryInterface(mChannel);

I don't think this will work - we don't create the channel until RecvAsyncOpen (and presumably this will also fail for the RecvConnectParent path as well).
Attachment #668672 - Flags: review?(josh) → review-
Posted patch v2-v3 diffSplinter Review
an interdiff that just shows the fix on top of v2.
Posted patch v3Splinter Review
nice catch--thanks.
Attachment #668672 - Attachment is obsolete: true
Attachment #669411 - Flags: review?(josh)
Comment on attachment 669411 [details] [diff] [review]

Review of attachment 669411 [details] [diff] [review]:

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +254,5 @@
>    rv = NS_LinkRedirectChannels(channelId, this, getter_AddRefs(mChannel));
>    LOG(("  found channel %p, rv=%08x", mChannel.get(), rv));
> +  if (mPBOverride != kPBOverride_Unset) {
> +    // redirected-to channel may not supports PB

Attachment #669411 - Flags: review?(josh) → review+

typo fixed.

Will land this on aurora once inbound shows green.
inbound is gree, code is e10s-only and blocking-basecamp, so no approval-mozilla-aurora? needed.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
please correct me if this is the wrong flag to set for "I landed this on aurora too"
(In reply to Jason Duell (:jduell) from comment #15)
> please correct me if this is the wrong flag to set for "I landed this on
> aurora too"
> status-firefox18: --- → fixed

That's right, although I'm not sure if we also need to set status-firefox19 → fixed.  (The merge tool should do this for us now that the flag is there, maybe?)
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:19.0) Gecko/20100101 Firefox/19.0
Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/20100101 Firefox/19.0

Works for me on Firefox 19 beta 5 (Build ID: 20130206083616) when using the link from comment 1.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.