bug 546573 creates transient about:blank a little more aggressively than we normally create them. In running browser-chrome tests, I ran head-on into browser/components/sessionstore/test/browser/browser_500328.js which does the following: * Creates a tab (with its embedded docshell) * Touches that tab (creates a transient about:blank document) * Loads about:blank explicitly (*replaces* the about:blank document) * Loads http://example.com explicitly from an onload handler (*replaces* the explicit about:blank document). * Calls history.pushState. The bug here is that the 2nd about:blank page tries to replace the session history entry (because it's a load normal for the same URI). However, the transient about:blank document doesn't have a session history entry (we explicitly set mOSHE to null at the end of CreateAboutBlankContentViewer). So, we fail to create the session history entry for the second about:blank document. Then, because the test sets the location of the tab from the onload handler of the previous load, we do the same thing again, and fail in the same way because we never successfully created a SHEntry for about:blank. As a note: we do create session history entries for about:blank; however, we tell them that they should not "persist", which has the effect that the session history manager replaces them when asked to add the next entry in the list.

This patch pains me a little, since all it does is add to the confusion that is docshell session history entry handling code; in particular, throughout the 11000 line file, we poke the session history entries almost randomly for effects far down the pipeline. It would be really nice if we could figure out some way to consolidate some of this logic...

Comment on attachment 459201 [details] [diff] [review] Proposed fix What if we don't have mOSHE but mLSHE? So what if we're loading a page, and try to load it again before the first page has been loaded?

I talked to smaug on IRC about his question but here's the logic that I ended up using: Looking through all *sets* of mOSHE in nsDocShell, we see: * The transient about:blank content viewer explicitly clears it. * Most other places have if (mLSHE) mOSHE = mLSHE, except * OnStateChange sets mOSHE = mLSHE without an explicit null check, however, in that case, we're loading a wcyiwyg channel and there's a comment in InternalLoad: // wyciwyg urls can only be loaded through history. Any normal load of which seems to suggest that mLSHE must be non-null. So, if we're in SetCurrentURI and mOSHE is null then: if mCurrentURI is null, this is the first page load and mLSHE should be null (can one ask a docshell to load a session history entry from a foreign docshell? That would be the only flaw in this statement). if mCurrentURI is non-null, then we must be at the about:blank content viewer, because otherwise, we'd have an mOSHE.

Comment on attachment 459201 [details] [diff] [review] Proposed fix This is needed by the blocking bug. It's hard for me to comment on risk, but I've tried to prove that this patch can only affect the one bad case that is biting my other bug.

Comment on attachment 459201 [details] [diff] [review] Proposed fix I marked this a blocker, no need for approval now. Please land...

I had to back this out. There was an assertion on the crashtest <parser/htmlparser/tests/crashtests/92788-1.html>. In this case, the docshell was for an iframe that was loading an error page. In that case, the error page code calls SetCurrentURI for the failed URL before OnNewURI, causing my assertion to trip. I'll re-check-in tomorrow with the assertion fixed to ignore error page loads, which are (AFIACT) the only way this can happen.

(In reply to comment #7) > I had to back this out. There was an assertion on the crashtest > <parser/htmlparser/tests/crashtests/92788-1.html>. In this case, the docshell > was for an iframe that was loading an error page. In that case, the error page > code calls SetCurrentURI for the failed URL before OnNewURI, causing my > assertion to trip. I'll re-check-in tomorrow with the assertion fixed to ignore > error page loads, which are (AFIACT) the only way this can happen. This can also happen on frames without session history, e.g. mailnews, no?

> I ran head-on into browser/components/sessionstore/test/browser > /browser_500328.js which does the following: You should be pleased to know that the fix you wrote for the painful test I wrote is now causing me pain in bug 673467. What goes around... :)