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

RESOLVED FIXED in Firefox 55

Status

Testing
Marionette
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

({regression})

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

Firefox Tracking Flags

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

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 months ago
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.
(Assignee)

Updated

2 months ago
Blocks: 1366199
(Assignee)

Comment 1

2 months ago
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)
(Assignee)

Updated

2 months ago
Blocks: 1291844
(Assignee)

Comment 3

2 months ago
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
(Assignee)

Comment 4

2 months ago
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.
(Assignee)

Updated

2 months ago
Attachment #8878488 - Flags: review?(ato)

Comment 5

2 months ago
mozreview-review
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+

Comment 6

2 months ago
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
Last Resolved: 2 months ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Comment 8

2 months ago
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 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/9a675b469134
status-firefox55: affected → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta]
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1260504
status-firefox54: --- → wontfix
(Assignee)

Updated

2 months ago
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)
(Assignee)

Comment 12

a month ago
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.