Closed Bug 1376558 Opened 6 years ago Closed 6 years ago

Should not migrate session for Firefox auto-refresh

Tracking

()

Status:

VERIFIED FIXED

Milestone:

Firefox 56

Tracking Flags:

Tracking

Status

firefox56

---

fixed

People

(Reporter: Fischer, Assigned: Fischer)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files)

Bug 1376558 - Should not migrate session for Firefox auto-refresh, 6 years ago Fischer [:Fischer] 59 bytes, text/x-review-board-request	MattN : review+	Details
windows_test_video 6 years ago Fischer [:Fischer] 29 bytes, text/plain		Details

Fischer [:Fischer]

Assignee

Description

•

6 years ago

Based on the 6/20 Photon Onboarding team meeting, we should show first run and don’t restore session (migrate sessionrestore) for Firefox auto-refresh (triggered by the sub-installer not manually chosen and triggered by user)

Fischer [:Fischer]

Assignee

Updated

•

6 years ago

Assignee: nobody → fliu

Status: NEW → ASSIGNED

Component: Installer → General

Priority: -- → P1

Whiteboard: [photon-onboarding]

Fischer [:Fischer]

Assignee

Updated

•

6 years ago

Blocks: 1268708

Fischer [:Fischer]

Assignee

Updated

•

6 years ago

Summary: Should not restore session for Firefox auto-refresh → Should not migrate session for Firefox auto-refresh

Comment hidden (mozreview-request)

hould not migrate session for Firefox auto-refresh

Review commit: https://reviewboard.mozilla.org/r/152658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/152658/

Francis Lee [:frlee]

Updated

•

6 years ago

Flags: qe-verify+

QA Contact: jwilliams

Target Milestone: --- → Firefox 56

Fischer [:Fischer]

Assignee

Comment 2

•

6 years ago

Comment on attachment 8881506 [details]
Bug 1376558 - Should not migrate session for Firefox auto-refresh,

Hi Matt,

From the search results[1][2], the MOZ_RESET_PROFILE_RESTART is set on 
- the safeMode dialog
- the about:support page
- the reset-profile notification
- UITour
to restart Firefox to refresh Firefox.

This patch simply removes the code of unsetting MOZ_RESET_PROFILE_RESTART in nsAppRunner.cpp. So that we could check its existence in FirefoxProfileMigrator.js before migrating session, then unset it.

[1] https://dxr.mozilla.org/mozilla-central/search?q=MOZ_RESET_PROFILE_RESTART&redirect=false
[2] https://dxr.mozilla.org/mozilla-central/search?q=ResetProfile.jsm&redirect=false

Thank you

Attachment #8881506 - Flags: review?(MattN+bmo)

Comment hidden (mozreview-request)

Comment on attachment 8881506 [details]
Bug 1376558 - Should not migrate session for Firefox auto-refresh,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152658/diff/1-2/

Fischer [:Fischer]

Assignee

Comment 4

•

6 years ago

(In reply to Fischer [:Fischer] from comment #3)
> Comment on attachment 8881506 [details]
> Bug 1376558 - Should not migrate session for Firefox auto-refresh,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/152658/diff/1-2/
Hi Matt,
Updated the patch according to our offline discussion. Use another environment variable to decide if session migration is needed. Thank you.

Fischer [:Fischer]

Assignee

Comment 5

•

6 years ago

Attached file windows_test_video — Details

Fischer [:Fischer]

Assignee

Comment 6

•

6 years ago

(In reply to Fischer [:Fischer] from comment #5)
> Created attachment 8882618 [details]: windows_test_video
Hi Matt,
Just tested this patch build on Windows. Please see attachment 8882618 [details]: windows_test_video.
In the video, we could see the about:welcomeback page(the old profile session migrated back) if the profile refresh was triggered through the refresh button on the troubleshooting page.
And no about:welcomeback(no old profile session migrated back) if the profile refresh was triggered through command[1] which is the case of the stub-installer.

[1] $ firefox.exe -reset-profile -migration

Thanks

Matthew N. [:MattN]

Comment 7

•

6 years ago

mozreview-review

Comment on attachment 8881506 [details]
Bug 1376558 - Should not migrate session for Firefox auto-refresh,

https://reviewboard.mozilla.org/r/152658/#review158876

::: browser/components/migration/FirefoxProfileMigrator.js:144
(Diff revision 2)
>    let dictionary = getFileResource(types.OTHERDATA, ["persdict.dat"]);
>  
> +  let session;
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +  if (env.get("MOZ_OLD_PROFILE_SESSION_MIGRATION")) {
> +    // XXX: Bug 1376558

Remove this line

::: browser/components/migration/FirefoxProfileMigrator.js:147
(Diff revision 2)
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +  if (env.get("MOZ_OLD_PROFILE_SESSION_MIGRATION")) {
> +    // XXX: Bug 1376558
> +    // We only want to restore the previous firefox session if the profile refresh was
> +    // triggered by user. The MOZ_OLD_PROFILE_SESSION_MIGRATION would be set when a user-triggered
> +    // profile refresh happend in nsAppRuner.cpp. Hence, we detec the MOZ_OLD_PROFILE_SESSION_MIGRATION

typo: happened

typo: detect

::: browser/components/migration/FirefoxProfileMigrator.js:147
(Diff revision 2)
> +  let env = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment);
> +  if (env.get("MOZ_OLD_PROFILE_SESSION_MIGRATION")) {
> +    // XXX: Bug 1376558
> +    // We only want to restore the previous firefox session if the profile refresh was
> +    // triggered by user. The MOZ_OLD_PROFILE_SESSION_MIGRATION would be set when a user-triggered
> +    // profile refresh happend in nsAppRuner.cpp. Hence, we detec the MOZ_OLD_PROFILE_SESSION_MIGRATION

Typo: nsAppRunner.cpp

::: toolkit/xre/nsAppRunner.cpp:2282
(Diff revision 2)
>  
>    if (EnvHasValue("MOZ_RESET_PROFILE_RESTART")) {
>      gDoProfileReset = true;
>      gDoMigration = true;
>      SaveToEnv("MOZ_RESET_PROFILE_RESTART=");
> +    // XXX: Bug 1376558

Remove this line since I don't think the bug number or XXX is necessary

::: toolkit/xre/nsAppRunner.cpp:2283
(Diff revision 2)
> +    // We only want to restore the previous firefox session if the profile refresh was
> +    // triggered by user. And if it was a user-triggered profile refresh
> +    // through, say, the safeMode dialog or the troubleshooting page, the MOZ_RESET_PROFILE_RESTART
> +    // env variable would be set. Hence we set MOZ_OLD_PROFILE_SESSION_MIGRATION here so that
> +    // Firefox profile migrator would migrate old session data later.

Don't mention the word Firefox in toolkit code

::: toolkit/xre/nsAppRunner.cpp:2288
(Diff revision 2)
> +    // We only want to restore the previous firefox session if the profile refresh was
> +    // triggered by user. And if it was a user-triggered profile refresh
> +    // through, say, the safeMode dialog or the troubleshooting page, the MOZ_RESET_PROFILE_RESTART
> +    // env variable would be set. Hence we set MOZ_OLD_PROFILE_SESSION_MIGRATION here so that
> +    // Firefox profile migrator would migrate old session data later.
> +    SaveToEnv("MOZ_OLD_PROFILE_SESSION_MIGRATION=1");

I think MOZ_RESET_PROFILE_MIGRATE_SESSION is a bit more consistent.

Attachment #8881506 - Flags: review?(MattN+bmo) → review+

Fischer [:Fischer]

Assignee

Comment 8

•

6 years ago

mozreview-review-reply

Comment on attachment 8881506 [details]
Bug 1376558 - Should not migrate session for Firefox auto-refresh,

https://reviewboard.mozilla.org/r/152658/#review158876

> Remove this line

Thanks updated

> typo: happened
> 
> typo: detect

Thanks updated

> Typo: nsAppRunner.cpp

Thanks updated

> Remove this line since I don't think the bug number or XXX is necessary

Thanks updated

> Don't mention the word Firefox in toolkit code

Thanks updated

> I think MOZ_RESET_PROFILE_MIGRATE_SESSION is a bit more consistent.

Thanks updated

Comment hidden (mozreview-request)

Comment on attachment 8881506 [details]
Bug 1376558 - Should not migrate session for Firefox auto-refresh,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/152658/diff/2-3/

Fischer [:Fischer]

Assignee

Comment 10

•

6 years ago

(In reply to Fischer [:Fischer] from comment #9)
> Comment on attachment 8881506 [details]
> Bug 1376558 - Should not migrate session for Firefox auto-refresh,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/152658/diff/2-3/
TRY looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5889653126bad2bbfea738bf6184ee33d58154dd

Keywords: checkin-needed

Fischer [:Fischer]

Assignee

Updated

•

6 years ago

Blocks: 1369255

Pulsebot

Comment 11

•

6 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/54b356661749
Should not migrate session for Firefox auto-refresh, r=MattN

Keywords: checkin-needed

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 12

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/54b356661749

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox56: --- → fixed

Resolution: --- → FIXED

Fischer [:Fischer]

Assignee

Comment 13

•

6 years ago

Hi Justin,

Here are the detailed expected behaviors for this bug:

1. Open the Nightly with an new profile
2. Make sure seeing the 1st-run page
3. Browse some websites and bookmark some websites
4. Open about:support in an new tab and click the refresh button to refresh Firefox
5. After restarting from the refresh, make sure seeing the about:welcomeback page
6. Restore the previous session and make sure previously-browsed websites are back
7. Make sure the bookmarked website exist
8. Close Firefox then reopen Firefox
9. Make sure the bookmarked website exist and Firefox runs as usual
10. Close Firefox and open the terminal
11. Launch Firefox with the command [1] or [2] to trigger auto-refresh
12. After Firefox starts up, make sure seeing the 1st-run page (not the about:welcomeback page)
13. Make sure the bookmarked website exist
14. Close Firefox then reopen Firefox
15. Make sure the bookmarked website exist and Firefox runs as usual
16. Done

[1] On Windows: <FOLDER_WHERE_FIREFOX_EXISTS>\firefox.exe -reset-profile -migration
[2] On Linux/Mac: <FOLDER_WHERE_FIREFOX_EXISTS>/firefox --reset-profile --migration

Thanks

Flags: needinfo?(jwilliams)

Justin [:JW_SoftvisionQA]

Updated

•

6 years ago

Depends on: 1381081

Justin [:JW_SoftvisionQA]

Updated

•

6 years ago

Flags: needinfo?(jwilliams)

Fischer [:Fischer]

Assignee

Updated

•

6 years ago

No longer depends on: 1381081

Fischer [:Fischer]

Assignee

Updated

•

6 years ago

Component: General → Migration

Justin [:JW_SoftvisionQA]

Comment 14

•

6 years ago

I have verified this issue is fixed.

Status: RESOLVED → VERIFIED

Fischer [:Fischer]

Assignee

Updated

•

6 years ago