529119 - Funky behavior with XUL error pages

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[u88484]

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 [:bzbarsky]]

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]

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)]

I've seen this bug before: Bug 528938.

Comment 4

[u88484]

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 6

[Boris Zbarsky [:bzbarsky]]

Bisect says:

The first bad revision is:
changeset:   34780:59ca9e3e4ef9
user:        Honza Bambas <[e-mail]>
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 [:bzbarsky]]

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 [:bzbarsky]]

It saddens me that we apparently had no reasonable error page tests.  :(

Comment 9

[Samuel Sidler (old account; do not CC)]

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

We need to fix this before we release 3.0.16/3.5.6

Comment 11

[cmtalbert]

(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)]

(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 [:bzbarsky]]

> That is set just after call to stop

Ah, good point.  OK.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: 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 [:bzbarsky]]

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)]

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 [:bzbarsky]]

> 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)]

(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)]

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 [:bzbarsky]]

The latter.

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 412879 [details] [diff] [review]
v1

This patch breaks the bug 302115 fix...

Comment 23

[Boris Zbarsky [:bzbarsky]]

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)]

Attachment: 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 [:bzbarsky]]

Comment on attachment 412888 [details] [diff] [review]
v2

Please add a test for this!

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: 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)]

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 [:bzbarsky]]

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)]

(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)]

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)]

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)]

Attachment: 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)]

(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)]

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 [:bzbarsky]]

Did we get a bug filed on the subframe history issues too, by the way?

Comment 36

[Honza Bambas (:mayhemer)]

Landed on 1.9.0 as part of bug 514232 checking on that branch. See bug 514232 comment 41.

Comment 37

[Honza Bambas (:mayhemer)]

(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)]

Honza: Did this land on 1.9.1 yet?

Comment 39

[Honza Bambas (:mayhemer)]

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)]

(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)]

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)]

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 44

[Boris Zbarsky [:bzbarsky]]

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)]

Have you tried to back out the fix and run the tests?

Comment 46

[Boris Zbarsky [:bzbarsky]]

Hmm... no.  Did I change the test in a way that no longer detects the bug?

Comment 47

[Honza Bambas (:mayhemer)]

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)]

Checked the tests are failing w/o this patch.

Comment 50

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.