Closed Bug 1373635 Opened 7 years ago Closed 7 years ago

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

Categories

(Remote Protocol :: Marionette, defect)

52 Branch
defect
Not set
normal

Tracking

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

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file)

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

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1186-1190

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.
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:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bb2ae032322c8579023a43f1e89bd7320d55b2e
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 on attachment 8878488 [details]
Bug 1373635 - Application profile has only be reset if explicitly requested.

https://reviewboard.mozilla.org/r/149852/#review154684
Attachment #8878488 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/274e6b6d1399
Application profile has only be reset if explicitly requested. r=ato
https://hg.mozilla.org/mozilla-central/rev/274e6b6d1399
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
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]
https://hg.mozilla.org/releases/mozilla-beta/rev/9a675b469134
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
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.
Flags: needinfo?(hskupin)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.