Last Comment Bug 673752 - Every error page fires onLocationChange twice
: Every error page fires onLocationChange twice
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: O. Atsushi (Torisugari)
:
:
Mentors:
Depends on: 750145
Blocks: 478927
  Show dependency treegraph
 
Reported: 2011-07-23 23:56 PDT by O. Atsushi (Torisugari)
Modified: 2012-04-29 19:12 PDT (History)
7 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix v1 (4.24 KB, patch)
2011-11-16 02:57 PST, O. Atsushi (Torisugari)
bugs: review-
Details | Diff | Splinter Review
Fix v2 (4.34 KB, patch)
2012-03-13 08:21 PDT, O. Atsushi (Torisugari)
bugs: review+
Details | Diff | Splinter Review

Description O. Atsushi (Torisugari) 2011-07-23 23:56:07 PDT
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.
Comment 1 O. Atsushi (Torisugari) 2011-11-16 02:57:42 PST
Created attachment 574859 [details] [diff] [review]
Fix v1



Note:

> mURIResultedInDocument = true;

is called right before this block.
http://hg.mozilla.org/mozilla-central/annotate/d51bd1645a2f/docshell/base/nsDocShell.cpp#l7390
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-01-09 10:42:26 PST
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*
Comment 3 O. Atsushi (Torisugari) 2012-01-10 00:39:32 PST
(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.
Comment 4 O. Atsushi (Torisugari) 2012-03-13 08:21:13 PDT
Created attachment 605405 [details] [diff] [review]
Fix v2

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

Anyway, approaching review comments.
Comment 5 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-04-23 05:11:19 PDT
Comment on attachment 605405 [details] [diff] [review]
Fix v2

Please ping me on IRC if reviewing takes time ;)
Comment 6 O. Atsushi (Torisugari) 2012-04-24 08:17:41 PDT
(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...
Comment 7 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-04-24 09:58:12 PDT
(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.
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-04-24 16:59:58 PDT
Thanks for the patch. Please include a patch description in the future, though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d6a254baa
Comment 9 Matt Brubeck (:mbrubeck) 2012-04-24 20:59:23 PDT
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.
Comment 10 O. Atsushi (Torisugari) 2012-04-25 07:04:49 PDT
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.
Comment 11 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-04-25 07:14:02 PDT
Which other patches were backed out?
Comment 12 Matt Brubeck (:mbrubeck) 2012-04-25 07:28:54 PDT
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
Comment 14 :Ehsan Akhgari 2012-04-25 07:42:47 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/b96c764c00b2
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-25 20:36:16 PDT
https://hg.mozilla.org/mozilla-central/rev/90b2927c5548

Note You need to log in before you can comment on or make changes to this bug.