Assertion failure: mDocument->IsXUL() || mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_INTERACTIVE || (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_UNINITIALIZED && NS_IsAboutBlank(mDocument->GetDocumentURI())) (Bad readystate), at layout/base/nsDocumentViewer.cpp:1017 I think what happened was that I was doing a Google search, then I clicked back expecting to be taken back to the Google home page, but forgot that Google Instant messes up session history quite badly, so I tried to load another page instead. Unfortunately I was pissed off that someone had deemed it necessary to try to crash my browser just because I had made a debug build so all I could be bothered to note down before I used gdb to jump to line 1018 was that the ready state was nsIDocument::READYSTATE_COMPLETE.

Henri?

(In reply to neil@parkwaycc.co.uk from comment #0) > Assertion failure: mDocument->IsXUL() || mDocument->GetReadyStateEnum() == > nsIDocument::READYSTATE_INTERACTIVE || (mDocument->GetReadyStateEnum() == > nsIDocument::READYSTATE_UNINITIALIZED && > NS_IsAboutBlank(mDocument->GetDocumentURI())) (Bad readystate), at > layout/base/nsDocumentViewer.cpp:1017 > > I think what happened was that I was doing a Google search, then I clicked > back expecting to be taken back to the Google home page, but forgot that > Google Instant messes up session history quite badly, so I tried to load > another page instead. Unfortunately I was pissed off that someone had deemed > it necessary to try to crash my browser just because I had made a debug > build Sorry. NS_ASSERTION is useless here, because it doesn't make mochitests go orange, so with NS_ASSERTION, this assertion would not help prevent the introduction of bugs. Hence an assertion that actually means something on mochitest. > so all I could be bothered to note down before I used gdb to jump to > line 1018 was that the ready state was nsIDocument::READYSTATE_COMPLETE. This might be a duplicate of bug 779100 or we have a yet-unknown bug about bfcache restore failing to result in |restoring| being true on line 1008.

(In reply to Henri Sivonen from comment #2) > Sorry. NS_ASSERTION is useless here, because it doesn't make mochitests go orange Fair enough.

I hit this again on a completely different computer. This time I was just navigating back to Google's home page (http://www.google.co.uk/) from an unrelated page that I had just loaded over it, so it's possible that there was a bfcache interaction although obviously the assert only fires if the docshell doesn't think that we're restoring. Again the document's readystate was already READYSTATE_COMPLETE. Stack: DocumentViewerImpl::LoadComplete+0x238 nsDocShell::EndPageLoad+0x21f nsDocShell::OnStateChange+0x735 nsDocLoader::DoFireOnStateChange+0x20c nsDocLoader::doStopDocumentLoad+0x109 nsDocLoader::DocLoaderIsEmpty+0x2ab nsDocLoader::OnStopRequest+0x399 nsLoadGroup::RemoveRequest+0x3f5 imgRequestProxy::RemoveFromLoadGroup+0x46 imgRequestProxy::OnStopRequest+0x10e imgStatusTracker::SendStopRequest+0x3d imgRequest::OnStopRequest+0x264 ProxyListener::OnStopRequest+0x42 nsStreamListenerTee::OnStopRequest+0xeb mozilla::net::nsHttpChannel::OnStopRequest+0x62f nsInputStreamPump::OnStateStop+0xeb nsInputStreamPump::OnInputStreamReady+0x9d nsInputStreamReadyEvent::Run+0x4a nsThread::ProcessNextEvent+0x34b NS_ProcessNextEvent_P+0x54 mozilla::ipc::MessagePump::Run+0xfd MessageLoop::RunInternal+0x4e MessageLoop::RunHandler+0x82 MessageLoop::Run+0x1d nsBaseAppShell::Run+0x50 nsAppShell::Run+0x47 nsAppStartup::Run+0x6a XREMain::XRE_mainRun+0xe41 XREMain::XRE_main+0x259 XRE_main+0x35

Hmm. So we get an image load when restoring from bfcache. What mechanism is supposed to prevent late image loads from re-firing onload?

That would normally be prevented by the mIsLoadingDocument check guarding the DocLoaderIsEmpty call in nsDocLoader::OnStopRequest and the same check in DocLoaderIsEmpty itself.

Assertion is reproducible loading http://www.walmart.com/ip/20532426?adid=22222222227014967638&wmlspartner=wlpa&wl0=&wl1=g&wl2=&wl3=14069093230&wl4=&wl5=pla&veh=sem&veh=dat on Windows XP Assertion is reproducible on Linux 32bit loading http://auto.howstuffworks.com/bugatti5.htm There are other examples if you need them. Though the urls appear to be platform specific in many ways, crash automation has reproduced on Linux, OS X, Windows on Aurora and Nightly.

This is my most frequent crash using a debug build for daily browsing. I've hit it 3 times in the past week or two (today on tbpl; see duplicate above for the first two). In the current state, this assertion should not be fatal.

I'm still unable to reproduce this problem. Considering comment 6 and searching for all instances of mIsLoadingDocument, the problem has to be that mIsLoadingDocument fails to get set to false in some cases. Part of the problem is that pieces of state such as mIsLoadingDocument are tracked on the loader/shell rather than on the document. It would be so much easier if we could trust readyState! (Maybe we already could and instead of asserting with the suspicion that readyState might still be broken we should start relying on readyState and get rid of flags like mIsLoadingDocument.) If I can't figure the problem out soon, I'll make the assertion non-fatal, but chances are the underlying bug here would have gone uncaught had the assertion been non-fatal.

So I could try setting mIsDocumentLoading to false every time mContentViewer changes, but I don’t really know the purpose of the IsBusy() checks, so I don’t know if those need to have a definition of being busy that outlives an mContentViewer change by a bit.

(In reply to Henri Sivonen (:hsivonen) from comment #10) > I'm still unable to reproduce this problem. I still see this in automation. I'll retest a bunch of urls and pick out a set which appear to be more reliable.

I have 14 or so urls where I've seen this assertion. These urls reproduce pretty well though it may take several tries (depending on ads?): http://www.myvideo.de/Videos_A-Z?searchWord=muskel%2Bmusik http://auto.howstuffworks.com/bugatti5.htm On OSX 10.7 the myvideo site gave me Assertion failure: inPrivateBrowsing == mFaviconLoadPrivate, at /work/mozilla/builds/nightly/mozilla/toolkit/components/places/AsyncFaviconHelpers.cpp:518 In automation I got: Assertion failure: mDocument->IsXUL() || mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_INTERACTIVE || (mDocument->GetReadyStateEnum() == nsIDocument::READYSTATE_UNINITIALIZED && NS_IsAboutBlank(mDocument->GetDocumentURI())) (Bad readystate), at c:/work/mozilla/builds/nightly/mozilla/layout/base/nsDocumentViewer.cpp:1017 If you have access to mpt, I can put this under a debugger for you.

Ehsan, see comment 13.

(In reply to Bob Clary [:bc:] from comment #13) > I have 14 or so urls where I've seen this assertion. These urls reproduce > pretty well though it may take several tries (depending on ads?): > > http://www.myvideo.de/Videos_A-Z?searchWord=muskel%2Bmusik > http://auto.howstuffworks.com/bugatti5.htm > > On OSX 10.7 the myvideo site gave me Assertion failure: inPrivateBrowsing == > mFaviconLoadPrivate, at > /work/mozilla/builds/nightly/mozilla/toolkit/components/places/ > AsyncFaviconHelpers.cpp:518 This is what I got in bug 795015. Was that when you opened a popup? Can you please test if my patch in that bug fixes this for you? > In automation I got: > > Assertion failure: mDocument->IsXUL() || mDocument->GetReadyStateEnum() == > nsIDocument::READYSTATE_INTERACTIVE || (mDocument->GetReadyStateEnum() == > nsIDocument::READYSTATE_UNINITIALIZED && > NS_IsAboutBlank(mDocument->GetDocumentURI())) (Bad readystate), at > c:/work/mozilla/builds/nightly/mozilla/layout/base/nsDocumentViewer.cpp:1017 > > If you have access to mpt, I can put this under a debugger for you. If this is not caused by opening a popup window, I would definitely like that (for the first assertion.)

I can’t work out what the *real* semantics of mIsLoadingDocument are, but clearly, mIsLoadingDocument does not really necessarily mean that the document is loading. I’m too afraid to remove mIsLoadingDocument, so I’m trying adding a check that trusts readyState whose semantics I’m familiar with. Let’s see how this goes on try.

Comment on attachment 665822 [details] [diff] [review] Trust readyState in nsDocLoader Makes the tree very orange. :-(

(In reply to Ehsan Akhgari [:ehsan] from comment #15) > > This is what I got in bug 795015. Was that when you opened a popup? Can > you please test if my patch in that bug fixes this for you? > Yes, it does appear to quickly open a popup and with your patch I get the Assertion failure: mDocument->IsXUL...

(In reply to comment #18) > (In reply to Ehsan Akhgari [:ehsan] from comment #15) > > > > > This is what I got in bug 795015. Was that when you opened a popup? Can > > you please test if my patch in that bug fixes this for you? > > > > Yes, it does appear to quickly open a popup and with your patch I get the > Assertion failure: mDocument->IsXUL... Phew! :-)

Related to bug 779430?

tar jxvf 779959.tar.bz2 firefox ./779959/testcase.html This testcase requires the testcase.js to be an external script and requires the the other script to be on the network. You may need to start/close the browser several times to see the assertion. I've seen this assertion on a total of 133 urls and on Firefox 16 and later.

So to fix this, we should move readyState from the document to a new object called nsNavigationLifeCycle or something like that. This object would be created when initially starting navigation in a docshell (i.e. when clicking a link) and the object would be passed through all redirects and eventually to the document that gets created. The doc loader should have a pointer to this object for the most recent navigation in the doc loader. Then the doc loader might have an actual chance of tracking whether it is loading a document.

https://hg.mozilla.org/mozilla-central/rev/d250db95c14b made this assertion non-fatal. I note this because it's been non-fatal in my tree since September, and I'm not sure how people have been using debug builds effectively for browsing in the interim...

> to a new object called nsNavigationLifeCycle or something like that Could we just put this on nsILoadInfo nowadays? Jonas, do we pass the loadinfo all the way through?

I don't really know what the problem in this bug is, or how nsNavigationLifeCycle would fix it. The way that nsILoadInfo works is that it's just a struct of extra "context" information which is provided whenever we do a network request. On redirect we clone the nsILoadInfo and copy over the appropriate information. The reason for the copy is so that we can keep redirect-chain information correct for both the pre-redirect and post-redirect channels correct at all times, which is useful in case we decide to not follow the redirect. If it would help matters here, we could create a refcounted nsNavigationLifeCycle object and have the nsILoadInfo point to that. And then copy over the reference whenever the nsILoadInfo is cloned.

(In reply to Jonas Sicking (:sicking) from comment #25) > I don't really know what the problem in this bug is, or how > nsNavigationLifeCycle would fix it. See https://github.com/servo/servo/pull/3714#issuecomment-67650099 Currently, we try to store state about a document load on the docloader before the document object for that load exists. The docloader is the wrong place, because more than one document lives in a single docloader over time, so the docloader, in some edge case (I haven't identified what case exactly), gets confused and holds the state for the wrong document. > The way that nsILoadInfo works is that it's just a struct of extra "context" > information which is provided whenever we do a network request. On redirect > we clone the nsILoadInfo and copy over the appropriate information. The > reason for the copy is so that we can keep redirect-chain information > correct for both the pre-redirect and post-redirect channels correct at all > times, which is useful in case we decide to not follow the redirect. > > If it would help matters here, we could create a refcounted > nsNavigationLifeCycle object and have the nsILoadInfo point to that. And > then copy over the reference whenever the nsILoadInfo is cloned. Is it possible that we end up creating two document objects from channels associated with two nsILoadInfos cloned from a common ancestor?

> Is it possible that we end up creating two document objects from channels associated with two > nsILoadInfos cloned from a common ancestor? There's two functions for cloning nsILoadInfos Clone(): http://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.h#57 CloneForNewRequest(): http://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.h#61 The former is used when a network request is redirected. The latter is used when we want to create a new network request which has similar properties as another network request. The reason we have two separate functions is because Clone() copies information like "has this network request been started" and "what URIs have this network request been redirected through so far". Whereas the latter for obvious reasons do not copy that information. Sounds like what you want to do is to not copy this loadstate stuff in CloneForNewRequest(), but do copy it in Clone().

bc: i was unable to reproduce, can you still reproduce this crash ?

As dbaron said this was converted into a non-fatal assertion. Now located at https://dxr.mozilla.org/mozilla-central/source/layout/base/nsDocumentViewer.cpp?from=nsDocumentViewer%3A%3ALoadComplete#961 I don't see it in any of our urls since our retest for 2015 urls nor is it reproducible in from the original urls using automation. The testcase fails to load with a malformed xml error. I defer to :hsivonen on whether to WFM this or not.

So did bug 779430 fix this one too? I guess it might make sense to close this one and open a new one if the assertion still fires.

(In reply to Olli Pettay [:smaug] from comment #30) > So did bug 779430 fix this one too? > I guess it might make sense to close this one and open a new one if the > assertion still fires. yeah i agree, closing as wfm