start_session inappropriately creates a new profile if not requested on shutdown with clean=True

RESOLVED FIXED in Firefox 55


2 years ago
2 years ago


(Reporter: whimboo, Assigned: whimboo)



52 Branch
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 fixed, firefox56 fixed)



(1 attachment)

Even the instance is not running we call `restart()` on it to get the process running again:

Providing no arguments will assume clean=True, and as such creates a new profile. Also it would try to close the application again, which at this stage is even not running. 

We should simply change this to just `start()`.

This behavior is present since Firefox 41 and was introduced by bug 1166033. Interestingly no single unit test catches it.
Blocks: 1366199
Also for a forced quit (without in_app=True argument) we invalidate the profile. I think we really should make this opt-in, and by default keep the current profile as we do for forced restarts. The API here is just broken.
Comment hidden (mozreview-request)
Blocks: 1291844
The try build shows failures with the initial safebrowsing test, and I'm not sure if this is related to artifact builds I have choosen on try, and bug 1371923 which is about to get fixed. I pushed another try for those failing test by not requesting artifact builds:
As I thought non-artifact builds do not cause the safebrowsing test to fail. All green. So I think we are good in getting this patch reviewed.
Attachment #8878488 - Flags: review?(ato)

Comment 5

2 years ago
Comment on attachment 8878488 [details]
Bug 1373635 - Application profile has only be reset if explicitly requested.
Attachment #8878488 - Flags: review?(ato) → review+

Comment 6

2 years ago
Pushed by
Application profile has only be reset if explicitly requested. r=ato
Last Resolved: 2 years ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
A bad regression we shipped for a while for the marionette harness package, and which prevents a test from using this method for testing. Given that it will be needed on beta, I request uplift of this test-only patch.
Whiteboard: [checkin-needed-beta]

Comment 9

2 years ago
status-firefox55: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
Duplicate of this bug: 1260504
status-firefox54: --- → wontfix
Depends on: 1375259
Were you thinking you might want this on ESR52 still? It'll need a rebased patch if the answer is yes.
Flags: needinfo?(hskupin)
Nothing actually utilizes it on esr52, so I don't think it's actually necessary to spend the time on a rebased patch.
status-firefox-esr52: affected → wontfix
Flags: needinfo?(hskupin)
You need to log in before you can comment on or make changes to this bug.