Closed
Bug 1390738
Opened 7 years ago
Closed 7 years ago
test_ext_contextual_identities.js is going to permafail when Gecko 57 merges to Beta on 2017-09-14
Categories
(WebExtensions :: General, defect, P1)
WebExtensions
General
Tracking
(firefox57+ verified)
VERIFIED
FIXED
mozilla57
People
(Reporter: RyanVM, Assigned: jkt)
References
Details
Attachments
(2 files)
[Tracking Requested - why for this release]: Permafailing tests on the next merge day. I'm guessing this tests implicitly assumes that NIGHTLY_BUILD is set and doesn't know what to do if it isn't. https://treeherder.mozilla.org/logviewer.html#?job_id=123438058&repo=try TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_with_permissions - [test_contextualIdentity_with_permissions : 187] Pref should now be initial state - true == false
Flags: needinfo?(jkt)
Assignee | ||
Comment 1•7 years ago
|
||
Can't immediately see why this is failing in the code I wrote however I will take this unless someone else knows of the fix from the web-ext team. Will consult with them all the same.
Assignee: nobody → jkt
Flags: needinfo?(jkt)
Comment 2•7 years ago
|
||
I think the problem is that the prefs that the API touches are only set on Nightly, and ExtensionPreferencesManager.jsm does not seem to handle non-existent preferences very well. The expedient fix would be to just add defaults for the privacy.userContext.* preferences to modules/libpref/init/all.js
Assignee | ||
Comment 3•7 years ago
|
||
I'm not sure how this is causing this issue, from what I can see all the prefs are set in: browser/app/profile/firefox.js Is that still going to be an issue? The failure that I think might be causing the others are related to not being able to get the following property: ReferenceError: reference to undefined property "app-temporary"" {file: "resource://gre/modules/addons/XPIProvider. jsm" line: 1569} However I'm not sure right now what would be causing that.
Flags: needinfo?(aswan)
Assignee | ||
Comment 4•7 years ago
|
||
Maybe that and other errors are actually ok. However the first fail is: TEST-UNEXPECTED-FAIL | xpcshell-remote.ini:toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js | test_contextualIdentity_with_permissions - [test_contextualIdentity_with_permissions : 187] Pref should now be initial state - true == false Which means the PreferenceManager isn't resetting the pref correctly.
Assignee | ||
Comment 5•7 years ago
|
||
This actually seems like the comment I left here: https://bugzilla.mozilla.org/show_bug.cgi?id=1354602#c35 When the pref is false initially it doesn't seem to be resetting itself in tests (however works in manual testing).
Assignee | ||
Comment 6•7 years ago
|
||
I have a feeling this is a timing issue of shutdown, when I tested it before I wasn't able to get it to pass. When I stick debugging in, it seems ADDON_UNINSTALL in ExtensionPreferencesManager.jsm after the tests await shutdown runs which would likely be a cause of the issue. Perhaps I can just observe change in the pref itself.
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8897921 [details] Bug 1390738 - Await for pref change if containers aren't enabled. https://reviewboard.mozilla.org/r/169166/#review174592 ::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:220 (Diff revision 1) > > + function waitForPrefChange(pref) { > + return new Promise(resolve => { > + function observeChange() { > + Services.prefs.removeObserver(pref, observeChange); > + resolve(Preferences.get(pref)); We're trying to get away from Preferences.jsm. But it doesn't look like you ever use the resolved value anyway so you can just resolve with no value here? ::: toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js:247 (Diff revision 1) > + await extension1.startup(); > + await extension1.awaitFinish("contextualIdentities"); > + equal(Services.prefs.getBoolPref(CONTAINERS_PREF), true, "Pref should now be enabled, whatever it's initial state"); > + const prefChange1 = waitForPrefChange(CONTAINERS_PREF); > + await extension1.unload(); > + // We explicitly have a pref that was set of, extensions turned it off nit: *off
Attachment #8897921 -
Flags: review?(aswan) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/587887d425c8 Await pref changes in contextual identities tests r=aswan
Keywords: checkin-needed
Updated•7 years ago
|
Flags: needinfo?(aswan)
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/587887d425c8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter | ||
Comment 12•7 years ago
|
||
Didn't work :( https://treeherder.mozilla.org/logviewer.html#?job_id=123747174&repo=try
Status: RESOLVED → REOPENED
Flags: needinfo?(jkt)
Resolution: FIXED → ---
Updated•7 years ago
|
Priority: -- → P2
Updated•7 years ago
|
Reporter | ||
Comment 13•7 years ago
|
||
Just a heads-up - due to the recently-announced changes to the Fx57 Beta cycle, the merge date is now a week from today. I've also verified that this issue is still present on current m-c tip.
Summary: test_ext_contextual_identities.js is going to permafail when Gecko 57 merges to Beta on 2017-09-20 → test_ext_contextual_identities.js is going to permafail when Gecko 57 merges to Beta on 2017-09-14
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(jkt)
Priority: P2 → P1
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
Hi Ryan, I wasn't able to replicate the test failure without manually faking the beta config by flipping the prefs. I added the amended review request which fixes the second test which was missed in the initial patch, this locally went from reproducing the issue to being fixed. Is there any way I can push this to try to get the same setup you did? Thanks
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 16•7 years ago
|
||
It seems mozreview might be a little screwy as the changes never got backed out. Attached is the patch to fix from the latest central.
Reporter | ||
Comment 17•7 years ago
|
||
(In reply to Jonathan Kingston [:jkt] from comment #15) > Is there any way I can push this to try to get the same setup you did? Applying https://hg.mozilla.org/try/rev/05559b5a212aa225f428e303ff3e5e17e68672f0 should suffice. But I'll kick off a Try push in a little bit with this patch included to see how it goes. Thanks!
Flags: needinfo?(ryanvm)
Reporter | ||
Comment 18•7 years ago
|
||
Comment on attachment 8905787 [details] [diff] [review] Bug 1390738 - Await for pref change if containers aren't enabled. r?aswan Looks like this one fixed it!
Attachment #8905787 -
Flags: feedback+
Assignee | ||
Comment 19•7 years ago
|
||
I have a feeling aswan is on PTO, could either of you check the test is ok here to land?
Flags: needinfo?(tomica)
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Updated•7 years ago
|
Attachment #8905787 -
Flags: review?(kmaglione+bmo)
Updated•7 years ago
|
Attachment #8905787 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 20•7 years ago
|
||
As discussed the checkin-needed is for the patch, I'm not sure if reviewboard patch will work. Either way they are the same. Thanks :kmag
Comment 21•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/5093b6067a20 Await for pref change if containers aren't enabled. r=aswan
Keywords: checkin-needed
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5093b6067a20
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Reporter | ||
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•