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

VERIFIED FIXED in Firefox 12

Status

()

Firefox
Session Restore
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Ginn Chen, Assigned: Alastair Robertson)

Tracking

Trunk
Firefox 12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 563767 [details] [diff] [review]
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.
(Assignee)

Updated

6 years ago
Attachment #563767 - Flags: feedback?(paul)
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?
Attachment #563767 - Flags: feedback?(paul)
Attachment #563767 - Flags: feedback?(gavin.sharp)
Attachment #563767 - Flags: feedback-
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?
Attachment #563767 - Flags: feedback?(gavin.sharp) → feedback+
(Assignee)

Comment 4

6 years ago
Created attachment 572918 [details] [diff] [review]
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!
Attachment #563767 - Attachment is obsolete: true
Attachment #572918 - Flags: review?(gavin.sharp)
(Assignee)

Comment 5

6 years ago
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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 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! :)
Attachment #572918 - Flags: review?(gavin.sharp) → review+
Duplicate of this bug: 711387
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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?
Assignee: nobody → alastair
Status: NEW → ASSIGNED
Hardware: x86 → All
It's another bug entirely and I r+ed the patch here, so it's clearly not blocking this patch.
(In reply to Gavin Sharp (use gavin@gavinsharp.com 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.
(Assignee)

Comment 11

5 years ago
Created attachment 582969 [details] [diff] [review]
Patch 1.2 - removed line from comments
Attachment #572918 - Attachment is obsolete: true
Keywords: checkin-needed
What's the checkin comment and user line for that patch?
(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
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Version: 4.0 Branch → Trunk
https://hg.mozilla.org/mozilla-central/rev/a5427ddeef60
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 12
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:12.0a1) Gecko/20120102 Firefox/12.0a1
Status: RESOLVED → VERIFIED
Blocks: 555529
You need to log in before you can comment on or make changes to this bug.