http://hg.mozilla.org/mozilla-central/annotate/ad1655c2e5b1/docshell/base/nsDocShell.cpp#l7513 >7513 // Create an shistory entry for the old load, if we have a channel >7514 if (failedChannel) { >7515 mURIResultedInDocument = PR_TRUE; >7516 OnLoadingSite(failedChannel, PR_TRUE, PR_FALSE); >7517 } else if (failedURI) { >7518 mURIResultedInDocument = PR_TRUE; >7519 OnNewURI(failedURI, nsnull, nsnull, mLoadType, PR_TRUE, PR_FALSE, >7520 PR_FALSE); >7521 } >7522 >7523 // Be sure to have a correct mLSHE, it may have been cleared by >7524 // EndPageLoad. See bug 302115. >7525 if (mSessionHistory && !mLSHE) { >7526 PRInt32 idx; >7527 mSessionHistory->GetRequestedIndex(&idx); >7528 if (idx == -1) >7529 mSessionHistory->GetIndex(&idx); >7530 >7531 nsCOMPtr<nsIHistoryEntry> entry; >7532 mSessionHistory->GetEntryAtIndex(idx, PR_FALSE, >7533 getter_AddRefs(entry)); >7534 mLSHE = do_QueryInterface(entry); >7535 } >7536 >7537 // Set our current URI >7538 SetCurrentURI(failedURI); >7539 >7540 mLoadType = LOAD_ERROR_PAGE; >7541 } >7542 >7543 PRBool onLocationChangeNeeded = OnLoadingSite(aOpenedChannel, PR_FALSE); These 30 liens of code calls SetCurrentURI(..) thrice and, what's worse, fires onLocationChange(...) twice (7516, 7519, 7538, 7543), though it's not causing a serious problem atm.

Note: > mURIResultedInDocument = true; is called right before this block. http://hg.mozilla.org/mozilla-central/annotate/d51bd1645a2f/docshell/base/nsDocShell.cpp#l7390

Comment on attachment 574859 [details] [diff] [review] Fix v1 > // Create an shistory entry for the old load, if we have a channel >- if (failedChannel) { >- mURIResultedInDocument = true; >- OnLoadingSite(failedChannel, true, false); >- } else if (failedURI) { >- mURIResultedInDocument = true; >- OnNewURI(failedURI, nsnull, nsnull, mLoadType, true, false, >- false); >- } >+#ifdef DEBUG >+ bool errorOnLocationChangeNeeded = >+#endif >+ OnNewURI(failedURI, failedChannel, nsnull, mLoadType, true, false, >+ false); So is it really guaranteed that failedURI or failedChannel is non-null here. The old code has null checks. >- SetCurrentURI(failedURI); >+ MOZ_ASSERT(!errorOnLocationChangeNeeded && >+ "We have to fire onLocationChange again."); Er what? expressiong && const char*

(In reply to Olli Pettay [:smaug] from comment #2) > So is it really guaranteed that failedURI or failedChannel is non-null here. > The old code has null checks. And the newer has the check only on debug build. You mean NS_ENSURE_TRUE is the better? > >- SetCurrentURI(failedURI); > >+ MOZ_ASSERT(!errorOnLocationChangeNeeded && > >+ "We have to fire onLocationChange again."); > > Er what? expressiong && const char* That is to leave a comment on the console on exit. I'm not too sure there's a smarter way, which is compatible with mfbt. BTW, I'm very busy this week. I'm sorry in advance.

I realized http://hg.mozilla.org/mozilla-central/rev/350305686094 was checked in in January. Anyway, approaching review comments.

Comment on attachment 605405 [details] [diff] [review] Fix v2 Please ping me on IRC if reviewing takes time ;)

(In reply to Olli Pettay [:smaug] from comment #5) > Please ping me on IRC if reviewing takes time ;) hmm, this is an extremely trivial bug, and I know you are always very busy. Generally speaking, it's somewhat difficult for me to send a ping. Besides, I had left out your review comment for 2 months this time...

(In reply to O. Atsushi (Torisugari) from comment #6) > (In reply to Olli Pettay [:smaug] from comment #5) > > Please ping me on IRC if reviewing takes time ;) > > hmm, this is an extremely trivial bug, this is not, which is why it is good if this gets landed early in the FF15 cycle.

Thanks for the patch. Please include a patch description in the future, though. https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d6a254baa

Sorry, I backed this out on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b96c764c00b2 because this changeset or another one from the same push caused 'make check' failures in Windows debug builds: TEST-PASS | jit_test.py -m -d -n | e:\builds\moz2_slave\m-in-w32-dbg\build\js\src\jit-test\tests\debug\Object-defineProperties-03.js command timed out: 300 seconds without output, attempting to kill https://tbpl.mozilla.org/php/getParsedLog.php?id=11178551&tree=Mozilla-Inbound We can use the Try server to figure out whether this patch caused the errors. If it does not cause the errors, we can re-land it.

If a test which uses onLocationChange or its bubbles (wrongly) expects docshell fires this event exactly twice, checkin may cause a timeout. http://mxr.mozilla.org/mozilla-central/source/js/src/jit-test/tests/debug/Object-defineProperties-03.js is not using onLocationChange, apparently. If the patch is really guilty, it will take some time to find out where to modify.

Which other patches were backed out?

Backing out this patch did not fix the hangs, so I re-landed it. (We ended up fixing the hangs by backing out some other patches.) https://hg.mozilla.org/integration/mozilla-inbound/rev/90b2927c5548

Backed out: https://hg.mozilla.org/mozilla-central/rev/b96c764c00b2