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)

defect

Tracking

(firefox57+ verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox57 + verified

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)
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)
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
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)
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.
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).
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 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+
Keywords: checkin-needed
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
Flags: needinfo?(aswan)
https://hg.mozilla.org/mozilla-central/rev/587887d425c8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Didn't work :(
https://treeherder.mozilla.org/logviewer.html#?job_id=123747174&repo=try
Status: RESOLVED → REOPENED
Flags: needinfo?(jkt)
Resolution: FIXED → ---
Priority: -- → P2
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
Flags: needinfo?(jkt)
Priority: P2 → P1
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)
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.
(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)
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+
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)
Attachment #8905787 - Flags: review?(kmaglione+bmo)
Attachment #8905787 - Flags: review?(kmaglione+bmo) → review+
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
Flags: needinfo?(tomica)
Flags: needinfo?(kmaglione+bmo)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/5093b6067a20
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.