654388 - "Restore Previous Session" button is always hidden if "Default Browser" dialog pops

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Ginn Chen]

Steps:

1) Make sure Firefox is not your default browser and you have default browser check for Firefox start-up. Also "about"home" is your home page.
2) Run Firefox, select "No" for default browser dialog, open several tabs, quit Firefox.
3) Run Firefox, select "No" for default browser dialog.
You can not see the "Restore Previous Session" button in about:home
4) Refresh about:home, now you see the button.
5) Restart Firefox, select "Yes" for default browser dialog.
You can not see the "Restore Previous Session" button in about:home
6) Refresh about:home, now you see the button.
7) Restart Firefox, now you don't have the default browser dialog because Firefox is the default browser. And you can see "Restore Previous Session" button.

Comment 1

[Alastair Robertson]

Attachment: Patch 1 - use setTimeout

The prompt asking whether to set Firefox as the default browser blocks the startup Javascript code until the user clicks one of the options. I've put the prompt in a setTimeout so it should only execute after the rest of the startup code is done.

Another solution would be to just move the code for the prompt to be after the code that initialises the session restore service, although this would mean it still blocks other pieces of startup code (but that might not matter).

--

The tests failed with and without my patch applied, so I'll try testing it again in a few days.

Comment 2

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

Comment on attachment 563767 [details] [diff] [review]
Patch 1 - use setTimeout

Ah I understand the problem here now (I didn't before, but this patch cleared it up). about:home still loads and we check sessionstore.canRestoreLastSession but since we haven't really init'd that's false even if it would become true.

I would actually much prefer just moving the code down since that's effectively what you're doing and setTimeout is misdirection that can lead to hard to track bugs.

I think moving the block down should be ok. We can still figure out if a session is being restored (I would add a comment about the about:home load needing the ss service init'd to work properly).

Gavin, you're more familiar with that whole delayedStartup process, does that sound reasonable?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 563767 [details] [diff] [review]
Patch 1 - use setTimeout

I think this is probably fine, as a spot fix. Would need to audit for any dependencies on when this code runs (don't think there are any), but using a timeout is more reliable than just moving it later (it's annoying and error prone to have to remember to explicitly keep it as the "last step" in startup).

Longer-term, we should get rid of this prompt entirely. A modal dialog on startup isn't a very good place to prompt - I think there are bugs on that?

Comment 4

[Alastair Robertson]

Attachment: Patch 1.1 - added comments

(In reply to Alastair Robertson from comment #1)
> The tests failed with and without my patch applied, so I'll try testing it
> again in a few days.

Took slightly longer than I intended (sorry), but the patch does pass the tests!

Comment 5

[Alastair Robertson]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #3)
> Would need to audit for any dependencies on when this code runs

Sorry, I didn't notice this before. What would this involve?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 572918 [details] [diff] [review]
Patch 1.1 - added comments

Sorry for the unreasonably long delay in response here, Alastair. This is a fine stop-gap for now, but if what the comment says is true ("ss service needs to be running before about:home is loaded"), then this isn't really a sufficient fix, since delayedStartup can still theoretically run after pages load. I think we should just remove that part of the comment. r=me with that.

I'd really like to investigate a few related fixes:

- bug 311605: move this code out of the browser.js window loading path and into nsBrowserGlue (runs once on startup)
- converting this modal dialog to some sort of non-modal notification
- having sessionstore functionality in general not depend on a window's delayedStartup having run (this probably involves re-working how sessionstore startup works, so it's probably not be trivial)

If you're interested in tackling the first one, I can guarantee quick review turnarounds, and eternal gratitude! :)

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #6)
> If you're interested in tackling the first one, I can guarantee quick review
> turnarounds, and eternal gratitude! :)

Gavin, is that a must-have to get this patch landed? Or can the proposed solutions be follow-up bugs?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

It's another bug entirely and I r+ed the patch here, so it's clearly not blocking this patch.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #9)
> It's another bug entirely and I r+ed the patch here, so it's clearly not
> blocking this patch.

Which means it can be checked-in? It seems that Alastair can't do it on his own, so can we please set the keyword if it's so? Thanks.

Comment 11

[Alastair Robertson]

Attachment: Patch 1.2 - removed line from comments

Comment 12

[Boris Zbarsky [:bzbarsky]]

What's the checkin comment and user line for that patch?

Comment 13

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

(In reply to Boris Zbarsky (:bz) from comment #12)
> What's the checkin comment and user line for that patch?

Thanks for checking but I took care of it.

https://hg.mozilla.org/integration/fx-team/rev/a5427ddeef60

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/a5427ddeef60

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120102 Firefox/12.0a1