Closed Bug 1373635 Opened 6 years ago Closed 6 years ago

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

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

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

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

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1373635 - Application profile has only be reset if explicitly requested.
59 bytes, text/x-review-board-request
ato
: review+
Details 
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.
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.
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.
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: 6 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]
https://hg.mozilla.org/releases/mozilla-beta/rev/9a675b469134
status-firefox55: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
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)
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.