Closed Bug 779100 Opened 7 years ago Closed 7 years ago

test_stricttransportsecurity.html manages to empty the load group for a docshell-generated about:blank document

Categories

(Core :: Networking: HTTP, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: hsivonen, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Steps to reproduce:

In nsDocumentViewer.cpp change

      MOZ_ASSERT(mDocument->IsXUL() || // readyState for XUL is bogus
                 mDocument->GetReadyStateEnum() ==
                   nsIDocument::READYSTATE_INTERACTIVE ||
                 // test_stricttransportsecurity.html has old-style
                 // docshell-generated about:blank docs reach this code!
                 (mDocument->GetReadyStateEnum() ==
                    nsIDocument::READYSTATE_UNINITIALIZED &&
                  NS_IsAboutBlank(mDocument->GetDocumentURI())),
                 "Bad readystate");

into

      MOZ_ASSERT(mDocument->IsXUL() || // readyState for XUL is bogus
                 mDocument->GetReadyStateEnum() ==
                   nsIDocument::READYSTATE_INTERACTIVE,
                 "Bad readystate");

and then run test_stricttransportsecurity.html.

Actual results:
mozilla::net::nsHttpChannel::ContinueAsyncRedirectChannelToHttps(unsigned int) removes the last request from a load group that belongs to a docshell-generated about:blank document. This is particularly bizarre, because docshell-generated about:blank documents are not supposed to have an HTTP connection associated with them. 

DocumentViewerImpl::LoadComplete(unsigned int) (/opt/Projects/mozilla-html5/layout/base/nsDocumentViewer.cpp:1015)
nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, unsigned int) (/opt/Projects/mozilla-html5/docshell/base/nsDocShell.cpp:6314)
nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, unsigned int) (/opt/Projects/mozilla-html5/docshell/base/nsDocShell.cpp:6144)
nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, unsigned int) (/opt/Projects/mozilla-html5/uriloader/base/nsDocLoader.cpp:1351)
nsDocLoader::doStopDocumentLoad(nsIRequest*, unsigned int) (/opt/Projects/mozilla-html5/uriloader/base/nsDocLoader.cpp:930)
nsDocLoader::DocLoaderIsEmpty(bool) (/opt/Projects/mozilla-html5/uriloader/base/nsDocLoader.cpp:822)
nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, unsigned int) (/opt/Projects/mozilla-html5/uriloader/base/nsDocLoader.cpp:707)
nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, unsigned int) (/opt/Projects/mozilla-html5/netwerk/base/src/nsLoadGroup.cpp:698)
mozilla::net::nsHttpChannel::ContinueAsyncRedirectChannelToHttps(unsigned int) (/opt/Projects/mozilla-html5/netwerk/protocol/http/nsHttpChannel.cpp:1676)
mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(unsigned int) (/opt/Projects/mozilla-html5/netwerk/protocol/http/nsHttpChannel.cpp:5754)
nsAsyncVerifyRedirectCallbackEvent::Run() (/opt/Projects/mozilla-html5/netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp:44)
nsThread::ProcessNextEvent(bool, bool*) (/opt/Projects/mozilla-html5/xpcom/threads/nsThread.cpp:624)
NS_ProcessNextEvent_P(nsIThread*, bool) (/opt/Projects/obj-debug/xpcom/build/nsThreadUtils.cpp:217)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/opt/Projects/mozilla-html5/ipc/glue/MessagePump.cpp:82)
MessageLoop::RunInternal() (/opt/Projects/mozilla-html5/ipc/chromium/src/base/message_loop.cc:209)
MessageLoop::RunHandler() (/opt/Projects/mozilla-html5/ipc/chromium/src/base/message_loop.cc:202)
MessageLoop::Run() (/opt/Projects/mozilla-html5/ipc/chromium/src/base/message_loop.cc:175)
nsBaseAppShell::Run() (/opt/Projects/mozilla-html5/widget/xpwidgets/nsBaseAppShell.cpp:165)
nsAppStartup::Run() (/opt/Projects/mozilla-html5/toolkit/components/startup/nsAppStartup.cpp:257)
XREMain::XRE_mainRun() (/opt/Projects/mozilla-html5/toolkit/xre/nsAppRunner.cpp:3787)
XREMain::XRE_main(int, char**, nsXREAppData const*) (/opt/Projects/mozilla-html5/toolkit/xre/nsAppRunner.cpp:3864)
XRE_main (/opt/Projects/mozilla-html5/toolkit/xre/nsAppRunner.cpp:3940)
do_main (/opt/Projects/mozilla-html5/browser/app/nsBrowserApp.cpp:160)
main (/opt/Projects/mozilla-html5/browser/app/nsBrowserApp.cpp:298)
__libc_start_main+0x000000ED  (/lib/x86_64-linux-gnu/libc.so.6)
_start (/opt/Projects/obj-debug/dist/bin/firefox-bin)

Expected results:
Expected networking code not to touch the load group of a docshell-generated about:blank document.

Additional information:
Even more strangely, if I patch nsDocShell.cpp to add an extra channel to the load group, the networking code still manages to cause the load group to be emptied and DocumentViewerImpl::LoadComplete to be called.
Looking into this.
Assignee: nobody → honzab.moz
As I thought.  The problem is that we first remove the old http channel from the loadgroup and only after that we call AsyncOpen on the new channel.  AsyncOpen adds a channel to its loadgroup.

Patch coming.
Status: NEW → ASSIGNED
Attached patch v1Splinter Review
- very simple patch
- make sure we first call AsyncOpen (to add it to its load group) on the new channel before we remove the old one from its load group

https://tbpl.mozilla.org/?tree=Try&rev=c2f032383c24
Attachment #648409 - Flags: review?(jduell.mcbugs)
Comment on attachment 648409 [details] [diff] [review]
v1

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1684,5 @@
> +    return rv;
> +}
> +
> +nsresult
> +nsHttpChannel::ContinueAsyncRedirectChannelToHttpsInternal(nsresult rv)

Can we stop our tradition of naming every piece of code we rip out into a new function by taking the existing name and adding "Handle", "Internal", and/or "Continue" to it?  "ContinueHandleAsyncFooInternal" says nothing descriptive.

How about something like "OpenRedirectToHttps"? I'd suggest "OpenHSTSRedirect", but we seem to avoid using "HSTS" (do we use this codepath for anything else?).

Otherwise looks good.
Attachment #648409 - Flags: review?(jduell.mcbugs) → review+
Re ContinueAsyncRedirectChannelToHttpsInternal:  That method is intended to be called only and only from ContinueAsyncRedirectChannelToHttps, so the name seems to me proper.  Since you didn't come up with a concrete name your self, I'd leave this as is.  It's clear at the first sight what the method's purpose is.
I came up with two concrete names :) but I don't want to argue about it--you can land as is.
Bug 765934 makes me think of a more proper name again...
(In reply to Jason Duell (:jduell) from comment #4)
> Comment on attachment 648409 [details] [diff] [review]
> v1
> > +nsHttpChannel::ContinueAsyncRedirectChannelToHttpsInternal(nsresult rv)
> 
> Can we stop our tradition of naming every piece of code we rip out into a

Changed to simple "OpenRedirectChannel".
https://hg.mozilla.org/mozilla-central/rev/60d3d0e84b2b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.