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.

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.

- 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

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.

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".

Comment on attachment 648409 [details] [diff] [review] v1 https://hg.mozilla.org/integration/mozilla-inbound/rev/60d3d0e84b2b (https://tbpl.mozilla.org/?tree=Try&rev=e98c3a832362)