Every error page fires onLocationChange twice

RESOLVED FIXED in mozilla15

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: O. Atsushi (Torisugari), Assigned: O. Atsushi (Torisugari))

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 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.
Version: unspecified → Trunk
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Updated

6 years ago
Attachment #574859 - Flags: review?(bugs)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

6 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*
Attachment #574859 - Flags: review?(bugs) → review-
(Assignee)

Comment 3

6 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.
(Assignee)

Comment 4

6 years ago
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.
Attachment #574859 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #605405 - Flags: review?(bugs)

Comment 5

5 years ago
Comment on attachment 605405 [details] [diff] [review]
Fix v2

Please ping me on IRC if reviewing takes time ;)
Attachment #605405 - Flags: review?(bugs) → review+
(Assignee)

Comment 6

5 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...
Blocks: 478927
Keywords: checkin-needed

Comment 7

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

Updated

5 years ago
Assignee: nobody → torisugari
Thanks for the patch. Please include a patch description in the future, though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c85d6a254baa
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla15
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.
Target Milestone: mozilla15 → ---
(Assignee)

Comment 10

5 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.
Severity: trivial → normal
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
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/c85d6a254baa
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Backed out: https://hg.mozilla.org/mozilla-central/rev/b96c764c00b2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/90b2927c5548
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 750145
You need to log in before you can comment on or make changes to this bug.