601024 - Still need to reload twice to reload page

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

from bug 600531 comment 11

it appears that this is not fully fixed.  It still takes two (2) presses of any refresh combo: Ctrl+r, Ctrl+f5 or F5 before the tab will reload.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

After looking at this quickly, we're still not removing the listener all the time, but we're also not hitting the conditions in onStateChange, so my current theory is that we're loading from cache and so STATE_IS_NETWORK might not be true (though that might have also been related to testing on the train with no internet). It makes sense though. It looks like reloads work for about: pages, which are never cached.

More testing in the morning.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

My new theory has nothing to do with loading from cache.

My bet is that we remove gRestoreTabsProgressListener before the last max_concurrent_tabs - 1 tabs have been restored (because we call restoreNextTab which ends up removing the listener based on window.__SS_tabsToRestore, which gets decremented in restoreTab instead of after it's been restored...)

Comment 3

[Andreas Eibach]

Paul, I think not only the keyboard is involved in this issue (F5/CTRL-R/CTRL-F5) but also the reload button (now (mis-)placed right of the address bar, must've been the first settings change I've done on my setup ;))

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

Scientific evidence supported my theory and so I fixed it.

No tests because it seems that the tests haven't actually hit this. But I applied and it worked for me.

Comment 5

[Tomas]

This bug should block b 7?

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 480771 [details] [diff] [review]
Patch v0.1

r=me

you have a test that specifically exercises the reload scenario, but it doesn't fail? it's pretty weird that you can manually reproduce it but not via the test. if you don't have a test that flexes the reload scenario in the way a user would, then i rescind my r+ above, until that codepath is exercised in a test.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #6)
> Comment on attachment 480771 [details] [diff] [review]
> Patch v0.1
> 
> r=me
> 
> you have a test that specifically exercises the reload scenario, but it doesn't
> fail? it's pretty weird that you can manually reproduce it but not via the
> test. if you don't have a test that flexes the reload scenario in the way a
> user would, then i rescind my r+ above, until that codepath is exercised in a
> test.

Bahhh I know why the test wasn't hitting this. test_reload2 (added in bug 600531) tests that we can reload after reloading each tab. It should be testing that you can reload after a normal restore of the tab (since that's the distinction that wasn't tested). That's not a bad test to have, but it doesn't actually exercise the same code path we want to test here.

I'll just add a new test that does a normal restore and then reloads each tab after that. That _should_ currently fail & _should_ pass with my patch above.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 480771 [details] [diff] [review]
Patch v0.1

Clearing review... as innocent as that looked it broke tests (caused issues with timing around detaching the progress listener).

I dug into that and fixed it only to expose more issues and break other tests because I was trying to reuse _resetTabRestoringState. It boiled down to completely overloading that method and trying to be too clever.

So instead I went back to the drawing board and made everything dumber. The refactoring is forthcoming in bug 602555. It will fix this bug.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Bug 602555 was fixed, so marking this fixed - landed http://hg.mozilla.org/mozilla-central/rev/bfa9a991f78e

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

in-testsuite+: the test for this was added with bug 602555.