Last Comment Bug 529119 - Funky behavior with XUL error pages
: Funky behavior with XUL error pages
Status: RESOLVED FIXED
: fixed1.9.0.16, regression
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- major with 1 vote (vote)
: ---
Assigned To: Honza Bambas (:mayhemer)
:
Mentors:
: 528938 529673 530338 (view as bug list)
Depends on:
Blocks: CVE-2009-3985
  Show dependency treegraph
 
Reported: 2009-11-16 14:36 PST by u88484
Modified: 2010-12-17 07:05 PST (History)
13 users (show)
mbeltzner: blocking1.9.2+
dveditz: blocking1.9.0.16+
dveditz: wanted1.9.0.x+
cmtalbert: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
alpha1+
beta4-fixed
.6+
.6-fixed


Attachments
v1 (1.02 KB, patch)
2009-11-17 09:41 PST, Honza Bambas (:mayhemer)
no flags Details | Diff | Splinter Review
v2 (2.49 KB, patch)
2009-11-17 11:07 PST, Honza Bambas (:mayhemer)
bzbarsky: review+
Details | Diff | Splinter Review
v2 + test (7.76 KB, patch)
2009-11-17 15:04 PST, Honza Bambas (:mayhemer)
samuel.sidler+old: approval1.9.1.6+
Details | Diff | Splinter 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] (9.48 KB, patch)
2009-11-18 06:55 PST, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Splinter Review

Description u88484 2009-11-16 14:36:15 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 14:39:30 PST
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...
Comment 2 u88484 2009-11-16 14:44:27 PST
Sorry BZ, I took that range from a post on mozillazine forums.  I assumed it was correct.
Comment 3 Ria Klaassen (not reading all bugmail) 2009-11-16 15:13:03 PST
I've seen this bug before: Bug 528938.
Comment 4 u88484 2009-11-16 15:14:30 PST
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.
Comment 5 u88484 2009-11-16 15:15:28 PST
*** Bug 528938 has been marked as a duplicate of this bug. ***
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 15:17:55 PST
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.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 15:19:11 PST
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?
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 15:19:45 PST
It saddens me that we apparently had no reasonable error page tests.  :(
Comment 9 Samuel Sidler (old account; do not CC) 2009-11-16 15:21:37 PST
(In reply to comment #6)
> Can't nominate for 1.9.1 blocking; ccing drivers.

bug 528867. I can nominate!
Comment 10 Daniel Veditz [:dveditz] 2009-11-16 15:27:25 PST
We need to fix this before we release 3.0.16/3.5.6
Comment 11 cmtalbert 2009-11-16 17:51:00 PST
(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.
Comment 12 Honza Bambas (:mayhemer) 2009-11-17 08:51:35 PST
(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 13 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 08:59:27 PST
> That is set just after call to stop

Ah, good point.  OK.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 09:14:53 PST
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).
Comment 15 Honza Bambas (:mayhemer) 2009-11-17 09:41:21 PST
Created attachment 412879 [details] [diff] [review]
v1

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.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 09:48:06 PST
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?
Comment 17 Honza Bambas (:mayhemer) 2009-11-17 09:57:40 PST
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 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 10:08:08 PST
> 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?
Comment 19 Honza Bambas (:mayhemer) 2009-11-17 10:10:10 PST
(In reply to comment #18)
> Do we have regression tests for the bug that added that LoadErrorPage chunk?

Bug 514232 or bug 302115?
Comment 20 Honza Bambas (:mayhemer) 2009-11-17 10:11:06 PST
For the former one, yes, but not in the suit, and for the letter, no, it seems we don't have one.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 10:11:46 PST
The latter.
Comment 22 Honza Bambas (:mayhemer) 2009-11-17 10:15:35 PST
Comment on attachment 412879 [details] [diff] [review]
v1

This patch breaks the bug 302115 fix...
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 10:20:31 PST
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.
Comment 24 Honza Bambas (:mayhemer) 2009-11-17 11:07:51 PST
Created attachment 412888 [details] [diff] [review]
v2

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.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 11:10:22 PST
Comment on attachment 412888 [details] [diff] [review]
v2

Please add a test for this!
Comment 26 Honza Bambas (:mayhemer) 2009-11-17 15:04:23 PST
Created attachment 412950 [details] [diff] [review]
v2 + test

Writing tests for this really is a pain...

bz, take a look at the test if you wish. I'll land this tomorrow morning.
Comment 27 Honza Bambas (:mayhemer) 2009-11-17 15:05:59 PST
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.
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2009-11-17 19:07:04 PST
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 Samuel Sidler (old account; do not CC) 2009-11-17 22:05:51 PST
(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?
Comment 30 Honza Bambas (:mayhemer) 2009-11-18 00:01:24 PST
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 Samuel Sidler (old account; do not CC) 2009-11-18 00:29:24 PST
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.
Comment 32 Honza Bambas (:mayhemer) 2009-11-18 06:55:21 PST
Created 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]

Ready to land

r=bzbarsky
Comment 33 Honza Bambas (:mayhemer) 2009-11-18 07:35:01 PST
(In reply to comment #28)
> And we should make it possible to usefully observe error page loads...

Reported bug 529531 on that.
Comment 34 Honza Bambas (:mayhemer) 2009-11-18 08:07:04 PST
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
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2009-11-18 08:10:36 PST
Did we get a bug filed on the subframe history issues too, by the way?
Comment 36 Honza Bambas (:mayhemer) 2009-11-18 08:46:36 PST
Landed on 1.9.0 as part of bug 514232 checking on that branch. See bug 514232 comment 41.
Comment 37 Honza Bambas (:mayhemer) 2009-11-18 09:32:04 PST
(In reply to comment #35)
> Did we get a bug filed on the subframe history issues too, by the way?

Bug 529559.
Comment 38 Samuel Sidler (old account; do not CC) 2009-11-18 17:04:12 PST
Honza: Did this land on 1.9.1 yet?
Comment 39 Honza Bambas (:mayhemer) 2009-11-19 08:27:46 PST
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.
Comment 40 Honza Bambas (:mayhemer) 2009-11-19 08:32:22 PST
(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.
Comment 41 Honza Bambas (:mayhemer) 2009-11-19 08:47:32 PST
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
Comment 42 Honza Bambas (:mayhemer) 2009-11-19 08:53:17 PST
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
Comment 43 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-19 09:49:16 PST
*** Bug 529673 has been marked as a duplicate of this bug. ***
Comment 44 Boris Zbarsky [:bz] (still a bit busy) 2009-11-19 17:47:35 PST
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
Comment 45 Honza Bambas (:mayhemer) 2009-11-20 07:01:44 PST
Have you tried to back out the fix and run the tests?
Comment 46 Boris Zbarsky [:bz] (still a bit busy) 2009-11-20 08:58:31 PST
Hmm... no.  Did I change the test in a way that no longer detects the bug?
Comment 47 Honza Bambas (:mayhemer) 2009-11-20 09:02:00 PST
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.
Comment 48 Honza Bambas (:mayhemer) 2009-11-21 11:48:16 PST
Checked the tests are failing w/o this patch.
Comment 49 Ria Klaassen (not reading all bugmail) 2009-11-22 10:29:01 PST
*** Bug 530338 has been marked as a duplicate of this bug. ***
Comment 50 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2010-01-22 14:28:47 PST
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.

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