Closed
Bug 529119
Opened 15 years ago
Closed 15 years ago
Funky behavior with XUL error pages
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | alpha1+ |
status1.9.2 | --- | beta4-fixed |
blocking1.9.1 | --- | .6+ |
status1.9.1 | --- | .6-fixed |
People
(Reporter: u88484, Assigned: mayhemer)
References
Details
(Keywords: fixed1.9.0.16, regression)
Attachments
(1 file, 3 obsolete files)
There is some funky behavior going on recently with XUL error pages. 'Try Again' button loads the previous page you were on and the back button on the navigation bar seems to pick a random spot in the history to load. STR: 1) Load http://www.google.com in a new tab 2) Go to http://www.23456789iuybh87g.com 3) Hit the 'Try Again' button, google loads 4) Repeat steps 1-2 5) Hit the back button on the navigation bar and some random page from the history loads and not google Regressed between 20091110 and 20091111 Pushlog for that range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f4ef40336cc6&tochange=2eb351cc47d3
Comment 1•15 years ago
|
||
Hmm. Are you sure about that range? Nothing in that range jumps out at me, while bug 514232 landed in the evening of 2009-11-11 and touched error page stuff...
Sorry BZ, I took that range from a post on mozillazine forums. I assumed it was correct.
BZ, I just confirmed myself that the regression range is between the 11/11 and 11/12 builds. http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2eb351cc47d3&tochange=6628da386550 Bug 514232 which you mentioned falls is the aforementioned range.
Summary: Funky behavior with XUL error pages → Server not found page "try again" button tries the wrong page. Back-button loads random page from history
Comment 6•15 years ago
|
||
Bisect says: The first bad revision is: changeset: 34780:59ca9e3e4ef9 user: Honza Bambas <honzab.moz@firemni.cz> date: Wed Nov 11 21:39:34 2009 +0100 summary: Bug 514232, r=bzbarsky so the range is just bogus. Can't nominate for 1.9.1 blocking; ccing drivers.
Blocks: CVE-2009-3985
Flags: blocking1.9.2?
Summary: Server not found page "try again" button tries the wrong page. Back-button loads random page from history → Funky behavior with XUL error pages
Comment 7•15 years ago
|
||
Honza, is the issue just that LoadErrorPage calls InternalLoad which calls Stop() and then that clears out mFailed*? Should we just be setting those after calling InternalLoad in LoadErrorPage?
Assignee: nobody → honzab.moz
Comment 9•15 years ago
|
||
(In reply to comment #6) > Can't nominate for 1.9.1 blocking; ccing drivers. bug 528867. I can nominate!
blocking1.9.1: --- → ?
Comment 11•15 years ago
|
||
(In reply to comment #8) > It saddens me that we apparently had no reasonable error page tests. :( Bz, Thanks for letting me know. We are a bit tapped out on the test dev front at the moment, but I'll add it to the list and see what we can do.
Flags: in-testsuite?
Assignee | ||
Comment 12•15 years ago
|
||
(In reply to comment #7) > Honza, is the issue just that LoadErrorPage calls InternalLoad which calls > Stop() and then that clears out mFailed*? Should we just be setting those > after calling InternalLoad in LoadErrorPage? It deletes the mFailed* members only when mLoadType is LOAD_ERROR_PAGE. That is set just after call to stop, so, what you say should not happen. Strange is that when I was carefully (manually) testing I took a great care to not break the history chain, i.e. from the wrong URL (for which we got an error page) it was able to go back to the previous page (google from the STR in the description for instance). Now it is not possible. I'm checking on my other machine that it was not broken by some other patch landed after mine.
Comment 14•15 years ago
|
||
OK, so what happens here is that in nsDocShell::OnNewURI we have mLSHE non-null, and in fact set to mOSHE. So if (!mLSHE && (mItemType == typeContent) && mURIResultedInDocument) { tests false, and we don't call AddToSessionHistory. And _that_ happens because LoadErrorPage sets mLSHE to the current sh entry (in this case the one for google).
Assignee | ||
Comment 15•15 years ago
|
||
Yes, I came to the same conclusion. It looked like I took a 'great care' only with the first version of the patch and not with the reviewed version. I was only sufficed by a passing test. We should probably enhance it with this scenario. I cannot check at the moment the fix it self is ok or not thanks bug 529119 :/ This is a patch that fixes the problem with back/forward and try again button not breaking bug 514232.
Attachment #412879 -
Flags: review?(bzbarsky)
Comment 16•15 years ago
|
||
Hmm. Why is that the right fix? This always nulls out the mLSHE on error page loads, no? Doesn't that undo whatever the code in LoadErrorPage is trying to do?
Assignee | ||
Comment 17•15 years ago
|
||
The 'if (failedChannel) {' etc. code block sets it back immediately, no? And it does set the right URL, the failed one. Or would you rather see a change to the OnNewURI method to set mLSHE even in case it has already been set, when we are loading an error page?
Comment 18•15 years ago
|
||
> The 'if (failedChannel) {' etc. code block sets it back immediately, no?
Even in the circumstances the LoadErrorPage code was trying to address (going back to error pages in history)?
I'm not sure what the right solution is; just trying to sort out what the pieces are, which parts we have regression tests for already, etc. Do we have regression tests for the bug that added that LoadErrorPage chunk?
Assignee | ||
Comment 19•15 years ago
|
||
(In reply to comment #18) > Do we have regression tests for the bug that added that LoadErrorPage chunk? Bug 514232 or bug 302115?
Assignee | ||
Comment 20•15 years ago
|
||
For the former one, yes, but not in the suit, and for the letter, no, it seems we don't have one.
Assignee | ||
Comment 22•15 years ago
|
||
Comment on attachment 412879 [details] [diff] [review] v1 This patch breaks the bug 302115 fix...
Attachment #412879 -
Attachment is obsolete: true
Attachment #412879 -
Flags: review?(bzbarsky)
Comment 23•15 years ago
|
||
The right fix here is probably to move the bug 302115 chunk to where the OnLoadingSite/etc got moved in bug 514232. That is, to between the OnLoadingSite/OnNewURI call and the SetCurrentURI call.
Assignee | ||
Comment 24•15 years ago
|
||
Yes, in other words, to move the whole block related to sh from nsDocShell::LoadErrorPage to nsDocShell::CreateContentViewer. Cool! Not breaking any of the bugs and fixing this one.
Attachment #412888 -
Flags: review?(bzbarsky)
Comment 25•15 years ago
|
||
Comment on attachment 412888 [details] [diff] [review] v2 Please add a test for this!
Attachment #412888 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 26•15 years ago
|
||
Writing tests for this really is a pain... bz, take a look at the test if you wish. I'll land this tomorrow morning.
Attachment #412888 -
Attachment is obsolete: true
Assignee | ||
Comment 27•15 years ago
|
||
Comment on attachment 412950 [details] [diff] [review] v2 + test The patch that caused this regression has already landed on 1.9.2 and 1.9.1. We should get this patch on both those branches.
Attachment #412950 -
Flags: approval1.9.2?
Attachment #412950 -
Flags: approval1.9.1.6?
Comment 28•15 years ago
|
||
Comment on attachment 412950 [details] [diff] [review] v2 + test s/fauly/faulty/ I don't like those setTimeouts: it seems like they can make this go orange on slow or loaded machines. Can you poll until the right thing happens instead? And we should make it possible to usefully observe error page loads...
Comment 29•15 years ago
|
||
(In reply to comment #27) > The patch that caused this regression has already landed on 1.9.2 and 1.9.1. We > should get this patch on both those branches. It will also cause a regression on 1.9.0 after bug 514232 lands there. Do you want to do a roll-up patch or tag this one for approval on 1.9.0?
Assignee | ||
Comment 30•15 years ago
|
||
That patch has not landed on 1.9.0 yet. I will adjust it before landing. (In reply to comment #28) > (From update of attachment 412950 [details] [diff] [review]) > s/fauly/faulty/ > > I don't like those setTimeouts: it seems like they can make this go orange on > slow or loaded machines. Can you poll until the right thing happens instead? > And we should make it possible to usefully observe error page loads... OK, will do that.
Comment 31•15 years ago
|
||
Comment on attachment 412950 [details] [diff] [review] v2 + test Pre-emptively approving for 1.9.1.6. a=ss But we'd like this to land on mozilla-central first.
Attachment #412950 -
Flags: approval1.9.1.6? → approval1.9.1.6+
Assignee | ||
Comment 32•15 years ago
|
||
Ready to land r=bzbarsky
Attachment #412950 -
Attachment is obsolete: true
Attachment #413068 -
Flags: review+
Attachment #412950 -
Flags: approval1.9.2?
Assignee | ||
Updated•15 years ago
|
Attachment #413068 -
Flags: approval1.9.2?
Assignee | ||
Comment 33•15 years ago
|
||
(In reply to comment #28) > And we should make it possible to usefully observe error page loads... Reported bug 529531 on that.
Assignee | ||
Comment 34•15 years ago
|
||
Comment on attachment 413068 [details] [diff] [review] v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41][Checkin 1.9.0 with bug 514232] http://hg.mozilla.org/mozilla-central/rev/05af3507e9c5
Attachment #413068 -
Attachment description: v2 + test v2 → v2 + test v2 [Checkin comment 34]
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 36•15 years ago
|
||
Landed on 1.9.0 as part of bug 514232 checking on that branch. See bug 514232 comment 41.
Assignee | ||
Comment 37•15 years ago
|
||
(In reply to comment #35) > Did we get a bug filed on the subframe history issues too, by the way? Bug 529559.
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Assignee | ||
Comment 39•15 years ago
|
||
Comment on attachment 413068 [details] [diff] [review] v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41][Checkin 1.9.0 with bug 514232] It's blocker, no need for approval.
Attachment #413068 -
Flags: approval1.9.2?
Assignee | ||
Comment 40•15 years ago
|
||
(In reply to comment #38) > Honza: Did this land on 1.9.1 yet? Going to do it now, I didn't catch up yesterday.
Assignee | ||
Comment 41•15 years ago
|
||
Comment on attachment 413068 [details] [diff] [review] v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41][Checkin 1.9.0 with bug 514232] http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9c52ac87808a
Attachment #413068 -
Attachment description: v2 + test v2 [Checkin comment 34] → v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40]
Assignee | ||
Updated•15 years ago
|
status1.9.2:
--- → final-fixed
Assignee | ||
Comment 42•15 years ago
|
||
Comment on attachment 413068 [details] [diff] [review] v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41][Checkin 1.9.0 with bug 514232] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/13efee696327
Attachment #413068 -
Attachment description: v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] → v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41]
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Updated•15 years ago
|
Keywords: fixed1.9.0.16
Assignee | ||
Updated•15 years ago
|
Attachment #413068 -
Attachment description: v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41] → v2 + test v2 [Checkin comment 34] [Checkin 1.9.2 comment 40] [Checkin 1.9.1 comment 41][Checkin 1.9.0 with bug 514232]
Comment 44•15 years ago
|
||
The 1.9.0 version of this fix sent the tree orange, because it relied on the error page title and the error page title is different on that branch. I've landed changes to address that: Checking in docshell/test/test_bug529119-1.html; /cvsroot/mozilla/docshell/test/test_bug529119-1.html,v <-- test_bug529119-1.html new revision: 1.4; previous revision: 1.3 Checking in docshell/test/test_bug529119-2.html; /cvsroot/mozilla/docshell/test/test_bug529119-2.html,v <-- test_bug529119-2.html new revision: 1.4; previous revision: 1.3 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a01764c72354 http://hg.mozilla.org/releases/mozilla-1.9.2/rev/189315c8f21a http://hg.mozilla.org/mozilla-central/rev/720ed723568b
Assignee | ||
Comment 47•15 years ago
|
||
Just to be sure, I am going to try right now. And thanks for the catch, btw. However I could not find it in the logs. It seems lost somewhere among leaks or such.
Assignee | ||
Comment 48•15 years ago
|
||
Checked the tests are failing w/o this patch.
blocking1.9.1: .6+ → ---
Updated•15 years ago
|
blocking1.9.1: --- → .6+
Comment 50•15 years ago
|
||
Marking the 14 bugs that are both: * nominated for blocking1.9.3:? * fixed on the 1.9.2 branch (according to status1.9.2) as blocking1.9.3:alpha1, so that we don't have to go through the nominations individually. They're all fixed already (so there's no work to do), and being fixed on 1.9.2 means they probably do block 1.9.3.
blocking2.0: ? → alpha1
You need to log in
before you can comment on or make changes to this bug.
Description
•