O. Atsushi (Torisugari) Assignee
	Description • 13 years ago

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.

	O. Atsushi (Torisugari) Assignee
	Comment 1 • 13 years ago

Note:

> mURIResultedInDocument = true;

is called right before this block.
http://hg.mozilla.org/mozilla-central/annotate/d51bd1645a2f/docshell/base/nsDocShell.cpp#l7390

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 13 years ago

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*

	O. Atsushi (Torisugari) Assignee
	Comment 3 • 13 years ago

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

	O. Atsushi (Torisugari) Assignee
	Comment 4 • 13 years ago

I realized http://hg.mozilla.org/mozilla-central/rev/350305686094 was checked in in January.

Anyway, approaching review comments.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 12 years ago

Comment on attachment 605405 [details] [diff] [review]
Fix v2

Please ping me on IRC if reviewing takes time ;)

	O. Atsushi (Torisugari) Assignee
	Comment 6 • 12 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 12 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 12 years ago

Thanks for the patch. Please include a patch description in the future, though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d6a254baa

	Matt Brubeck (:mbrubeck)
	Comment 9 • 12 years ago

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.

	O. Atsushi (Torisugari) Assignee
	Comment 10 • 12 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 12 years ago

Which other patches were backed out?

	Matt Brubeck (:mbrubeck)
	Comment 12 • 12 years ago

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

	(no longer active)
	Comment 13 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/c85d6a254baa

	(no longer active)
	Comment 14 • 12 years ago

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

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 15 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/90b2927c5548