Closed Bug 1457745 Opened 6 years ago Closed 6 years ago

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_pluginInfoURL.js

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(thunderbird60 fixed, thunderbird61 fixed)

RESOLVED FIXED
Thunderbird 61.0
Tracking Status
thunderbird60 --- fixed
thunderbird61 --- fixed

People

(Reporter: jorgk-bmo, Unassigned)

Details

(Whiteboard: [Thunderbird-testfailure: X all])

Attachments

(1 file)

TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_pluginInfoURL.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_pluginInfoURL.js | test_infoURL_missing - [test_infoURL_missing : 74] Should be using fallback when no infoURL tag is available. - "https://blocklist.addons.mozilla.org/en-US/xpcshell/blocked/test_plugin_noInfoURL" === "https://blocked.cdn.mozilla.net/test_plugin_noInfoURL.html"

M-C last good: 8b2c1fc3d6c348f053fe33a478fa3b1ddb
M-C first bad: 08f68e2c892cadc4035ecbfbf3529f32d4
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8b2c1fc3d6c348f053fe33a478fa3b1ddb&tochange=08f68e2c892cadc4035ecbfbf3529f32d4

Looks like bug 1456515 last changed test_pluginInfoURL.js.

Gijs and Kris, what's going wrong in the M-C test when run in TB?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)
Whiteboard: [Thunderbird-testfailure: X all]
https://dxr.mozilla.org/mozilla-central/search?q=https%3A%2F%2Fblocked.cdn.mozilla.net%2F&=mozilla-central

seems like this is not a test domain but the "normal" greprefs one. Presumably TB/SM override this?

If TB/SM have a good reason to not use the CDN for blocklist items (:TheOne might know?) then presumably the way to fix this would be to update the test to pushPrefEnv a different value for that pref so we don't implicitly rely on the shipped value -- but off-hand it would seem to me that TB/SM should be using the same CDN. Andreas?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(awagner)
I'm sorry I don't know what this bug is about. Can you please add more context?
Flags: needinfo?(awagner)
(In reply to Andreas Wagner [:TheOne] [use NI] from comment #2)
> I'm sorry I don't know what this bug is about. Can you please add more
> context?

From where do you/we expect TB/SM to get the informational pages when items are blocklisted? On m-c, it seems we use `https://blocked.cdn.mozilla.net/` . TB seems to be using "https://blocklist.addons.mozilla.org/" . Is that expected?
Flags: needinfo?(awagner)
The bug is about the Xpcshell test test_pluginInfoURL.js failing when run in the Thunderbird environment.

I think Gijs is suggesting that two(?) preferences are at play here:
extensions.blocklist.detailsURL and extensions.blocklist.itemURL.

We use
pref("extensions.blocklist.detailsURL", "https://addons.mozilla.org/%LOCALE%/%APP%/blocked/");
pref("extensions.blocklist.itemURL", "https://blocklist.addons.mozilla.org/%LOCALE%/%APP%/blocked/%blockID%");

extensions.blocklist.url is different, too:
pref("extensions.blocklist.url", "https://blocklist.addons.mozilla.org/blocklist/3/%APP_ID%/...

Blame shows that they were changed in 2011 and 2014:
https://hg.mozilla.org/comm-central/rev/728871c36cf7
https://hg.mozilla.org/comm-central/rev/69dd558de5f9

So should we just delete the prefs and go with what ever M-C does? How will that be in the future (RSN(TM)) when we run our own add-ons site?

Personally, I'd like to have the test failure cleared, so how can we achieve this quickly while we're working on an overall strategy?
Flags: needinfo?(sancus)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Jorg K (GMT+1) from comment #4)
> The bug is about the Xpcshell test test_pluginInfoURL.js failing when run in
> the Thunderbird environment.
> 
> I think Gijs is suggesting that two(?) preferences are at play here:
> extensions.blocklist.detailsURL and extensions.blocklist.itemURL.
> 
> We use
> pref("extensions.blocklist.detailsURL",
> "https://addons.mozilla.org/%LOCALE%/%APP%/blocked/");
> pref("extensions.blocklist.itemURL",
> "https://blocklist.addons.mozilla.org/%LOCALE%/%APP%/blocked/%blockID%");
> 
> extensions.blocklist.url is different, too:
> pref("extensions.blocklist.url",
> "https://blocklist.addons.mozilla.org/blocklist/3/%APP_ID%/...
> 
> Blame shows that they were changed in 2011 and 2014:
> https://hg.mozilla.org/comm-central/rev/728871c36cf7
> https://hg.mozilla.org/comm-central/rev/69dd558de5f9
> 
> So should we just delete the prefs and go with what ever M-C does?

I don't know, that's why I'm asking Andreas...

> How will
> that be in the future (RSN(TM)) when we run our own add-ons site?
> 
> Personally, I'd like to have the test failure cleared, so how can we achieve
> this quickly while we're working on an overall strategy?

I suggested how to fix this in comment #1...

(In reply to :Gijs (he/him) from comment #1)
> then presumably the way to fix this would be to update the test
> to pushPrefEnv a different value for that pref so we don't implicitly rely
> on the shipped value


but given that m-c is using a CDN, I'm inclined to think TB should be using that too, unless Andreas suggests otherwise.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs (he/him) from comment #3)

> From where do you/we expect TB/SM to get the informational pages when items
> are blocklisted? On m-c, it seems we use `https://blocked.cdn.mozilla.net/`
> . TB seems to be using "https://blocklist.addons.mozilla.org/" . Is that
> expected?

I am not sure this is still working for Tb/Sm. The URL changed in https://bugzilla.mozilla.org/show_bug.cgi?id=1341332 and it seems like at some point we changed or dropped the redirects.
(In reply to Andreas Wagner [:TheOne] [use NI] from comment #6)
> (In reply to :Gijs (he/him) from comment #3)
> 
> > From where do you/we expect TB/SM to get the informational pages when items
> > are blocklisted? On m-c, it seems we use `https://blocked.cdn.mozilla.net/`
> > . TB seems to be using "https://blocklist.addons.mozilla.org/" . Is that
> > expected?
> 
> I am not sure this is still working for Tb/Sm. The URL changed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1341332 and it seems like at
> some point we changed or dropped the redirects.

OK. Then I think TB should drop its prefs so it uses the m-c ones. AFAICT SM doesn't have this set of prefs in the first place, only TB...
Flags: needinfo?(awagner)
I am also just inferring from what I read in https://bugzilla.mozilla.org/show_bug.cgi?id=1341332 which I found from blaming that pref in hg).

Maybe this was done around the move to kinto? The timing would fit. I don't have any further information and so I am not sure I am the right person to give guidance here, sorry.

Perhaps :leplatrem knows more?
Flags: needinfo?(mathieu)
Removing those preferences, we will now use the Mozilla default from here:
https://searchfox.org/mozilla-central/rev/08df4e6e11284186d477d7e5b0ae48483ecc979c/modules/libpref/init/all.js#2526

Services.kinto.* appear to be deprecated as well, so I removed them while I was here.
Attachment #8971969 - Flags: review?(sancus)
Attachment #8971969 - Flags: review?(mkmelin+mozilla)
Attachment #8971969 - Flags: review?(mathieu)
(In reply to Jorg K (GMT+1) from comment #4)
> So should we just delete the prefs and go with what ever M-C does? How will
> that be in the future (RSN(TM)) when we run our own add-ons site?
> 
> Personally, I'd like to have the test failure cleared, so how can we achieve
> this quickly while we're working on an overall strategy?

These URLs will need to point at addons.thunderbird.net within the next couple of months, certainly. That doesn't necessarily impact any change now. It does mean that Mozilla will need to add an additional rewrite to thunderbird.net at the CDN, which I'm not certain how difficult it is to do, unless we are OK writing off versions that get this change as unable to connect to the blocklist. Maybe less important if it only goes in a couple betas/nightlies, but a problem if it's in 60.0 release.

I could also put in redirects at live.thunderbird.net so that we only need to change these once, as long as redirects are acceptable here.
Flags: needinfo?(sancus)
Comment on attachment 8971969 [details] [diff] [review]
1457745-remove-bl-prefs.patch

I've discussed this with Andrei on IRC. I'll just go ahead and land this as a bustage fix. Soon TB will need it's own blocklist URL, then we put the preferences back and also submit a patch to fix the test.
Flags: needinfo?(mathieu)
Attachment #8971969 - Flags: review?(sancus)
Attachment #8971969 - Flags: review?(mkmelin+mozilla)
Attachment #8971969 - Flags: review?(mathieu)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/323c8a1be19c
removed unnecessary and faulty blocklist preferences. rs=bustage-fix
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 61.0
Attachment #8971969 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/df13f2b8f71c
Follow-up: removed more deprecated kinto preferences. r=me DONTBUILD
For the record:
We added these kinto prefs in Feb/March 2016:
https://hg.mozilla.org/comm-central/rev/57ada76a9c40
https://hg.mozilla.org/comm-central/rev/9ebe6f187f59
https://hg.mozilla.org/comm-central/rev/16b7ef1fb002

M-C renamed and moved them from FF to lib in April of 2016:
https://hg.mozilla.org/mozilla-central/rev/c6c57d394549
https://hg.mozilla.org/mozilla-central/rev/a5aad78f70ea

So basically TB has been wrong for two years now, but since M-C's lib provided the correct preferences, no one noticed :-(
> Maybe this was done around the move to kinto? The timing would fit. I don't have any further information and so I am not sure I am the right person to give guidance here, sorry.
> Perhaps :leplatrem knows more?

Indeed, when the source of truth changed from AMO to Kinto we redirected the URL. However, I was pretty sure we had put a redirection :(
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: