1476177 - Addons with platform specific .xpi files don't end up with a sourceURI attribute

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Some addons provide a unique xpi file for each desktop platform - eg:

> https://addons.mozilla.org/en-US/firefox/addon/emoji-box/

where the metadata is parsed via:
> https://services.addons.mozilla.org/api/v3/addons/search/?guid=%7B369b4c1b-94a5-46ce-9301-d18120875adc%7D&lang=en-US

The metadata returned for this addon has in aEntry.current_version.files:

> {"id":973439,"platform":"linux","url":"https://addons.mozilla.org/firefox/downloads/file/973439/emoji_box-1.2.86.1405-an+fx-linux.xpi?src="},
> {"id":973440,"platform":"mac","url":"https://addons.mozilla.org/firefox/downloads/file/973440/emoji_box-1.2.86.1405-an+fx-mac.xpi?src="},
> {"id":973441,"platform":"windows","url":"https://addons.mozilla.org/firefox/downloads/file/973441/emoji_box-1.2.86.1405-an+fx-windows.xpi?src="},

The AddonRepository, at https://searchfox.org/mozilla-central/rev/6f86cc3479f80ace97f62634e2c82a483d1ede40/toolkit/mozapps/extensions/internal/AddonRepository.jsm#585, tries to find the correct platform to use for the sourceURI attribute. 

However, for me, Services.appinfo.OS.toLowerCase() returns "winnt", but the code is checking for "windows".

Sync then declines to sync this addon as it can't confirm the addon source is trusted, leading to bug 1467904.

The docs for Services.appinfo.OS indicates it "is taken from the OS_TARGET configure variable", and if that's true, it appears likely this is broken on all platforms - searching the tree for OS_TARGET implies it could be, at least, 'WINNT', 'Darwin', 'FreeBSD', 'OpenBSD', and 'NetBSD'. However, I've only confirmed it on Windows.

Best I can tell, this was introduced in bug 1402064, so is in 60+

Comment 1

[Mark Hammond [:markh] [:mhammond]]

It appears many .xpi files on AMO are now setup this way - has the packaging on AMO changed recently? If so it might explain a different Sync bug where enabled states aren't syncing, which could be explained if the addons was synced when there was a single file with platform=="all", but was since repacked with multiple platform-specific files.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Hey Andrew and Kris, any thoughts on this? It would be nice to fix addon sync as soon as we can.

Comment 3

[Andrew Swan [:aswan]]

This looks like a long-standing bug :/  Here is the equivalent code from before bug 1402064:
https://searchfox.org/mozilla-central/rev/45a6d267615b9e1236c7b41bf38af02faa10bef0/toolkit/mozapps/extensions/internal/AddonRepository.jsm#1013

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Is it possible that how AMO bundles addons has changed recently? I'm seeing some addons that don't appear to need platform specific xpi files have them.

IOW, I'm suspecting that some addons that previously had 1 file with platform="all" now have 3 files.

Comment 5

[Andrew Swan [:aswan]]

Andrew, does anything about comment 4 ring a bell?

Comment 6

[Kris Maglione [:kmag]]

(In reply to Mark Hammond [:markh] from comment #4)
> Is it possible that how AMO bundles addons has changed recently? I'm seeing
> some addons that don't appear to need platform specific xpi files have them.
> 
> IOW, I'm suspecting that some addons that previously had 1 file with
> platform="all" now have 3 files.

Do you have a particular example?

This is what AMO has always done. Whenever an add-on specifies a platform for a version, it gets separate files for every platform it supports. It's been that way since the dawn of time... or, well, the dawn of AMO.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

I don't have an example of an addon which I suspect has changed recently. However, https://github.com/mozilla/contain-facebook is an addon which surprises me with the multiple files - ie, https://services.addons.mozilla.org/api/v3/addons/search/?guid=@contain-facebook&lang=en-US shows platform specific xpi files.

Comment 8

[Kris Maglione [:kmag]]

That add-on is listed as explicitly supporting 3 separate platforms rather than just supporting all platforms.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Fair enough - so maybe this isn't actually a regression - but we've certainly seen a spike in error reports around addons recently, all of which can be traced to the addon having multiple platforms. I'm reluctant to drop checking the sourceURI as the check is there to keep users safe. What do you recommend we do?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

(In reply to Andrew Swan [:aswan] from comment #3)
> This looks like a long-standing bug :/  Here is the equivalent code from
> before bug 1402064:
> https://searchfox.org/mozilla-central/rev/
> 45a6d267615b9e1236c7b41bf38af02faa10bef0/toolkit/mozapps/extensions/internal/
> AddonRepository.jsm#1013

Ah - I think I know what's going on - that code uses a lower API level and looks at the XML.

> curl -v --header "Accept: text/html, application/xhtml+xml, application/xml;q=0.9, */*;q=0.8" "https://services.addons.mozilla.org/en-US/firefox/api/1.5/search/guid:@contain-facebook?src=firefox&appOS=WINNT&appVersion=60.0a1"

Gives back XML with:

>  <install os="Linux" ... </install>
>  <install os="Darwin" ... </install>
>  <install os="WINNT" ... </install>

Note the difference in values, meaning the old code did work.

Comment 11

[Andrew Williamson [:eviljeff]]

I don't think there's anything wrong on the API side here - the individual file details are still there (hashes, filenames, etc); it's just returns 'nicer' constant values (e.g. "windows") rather than the toolkit constants ("WINNT") now.

Comment 12

[Andrew Swan [:aswan]]

(In reply to Andrew Williamson [:eviljeff] from comment #11)
> I don't think there's anything wrong on the API side here - the individual
> file details are still there (hashes, filenames, etc); it's just returns
> 'nicer' constant values (e.g. "windows") rather than the toolkit constants
> ("WINNT") now.

erp, that's the problem I guess.

Comment 14

[Shane Caraveo (:mixedpuppy)]

> However, for me, Services.appinfo.OS.toLowerCase() returns "winnt", but the
> code is checking for "windows".

Before being introduced to this bug, I did a patch.  I tested on linux, appinfo.OS gave me linux.  Thought I'd tested on windows, but just re-verified, I also get winnt.

Comment 15

[Andrew Swan [:aswan]]

Attachment: Bug 1476177 Fix platform matching in AMO metadata requests

Review commit: https://reviewboard.mozilla.org/r/260806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260806/

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

https://reviewboard.mozilla.org/r/260806/#review268006

In Bug 1478669 I had modified the tests to make sure the platform names (at least mac) got some coverage.  I think you should pull that over here, maybe add windows as well.

Comment 17

[Andrew Swan [:aswan]]

I saw that you made some test changes, but I don't think there was actually any expanded test coverage in that patch?
We can't test against live AMO in automation, I think the chances of a regression in this area are quite small, but if there is one, it will only realistically ever be caught by manual testing (or more likely, somebody noticing something is broken as this bug was discovered)

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #17)
> I saw that you made some test changes, but I don't think there was actually
> any expanded test coverage in that patch?

The test data has "all" for platform, so the platform specific check itself is never tested.  I know it's kind of trivial, but it's also a trivial change in the test.   In this particular case, and certainly in hindsight, if the original test data added in bug 1402064 had the platform sections, this would have been caught.  What cant be caught with dummy data is a later change in AMO.

Dummy datasets could be fetched directly from AMO and saved into a static files used for local tests.

Comment 19

[Andrew Swan [:aswan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> (In reply to Andrew Swan [:aswan] from comment #17)
> > I saw that you made some test changes, but I don't think there was actually
> > any expanded test coverage in that patch?
> 
> The test data has "all" for platform, so the platform specific check itself
> is never tested.

The patches on bug 1478669 didn't actually exercise that code path, but I'll add something.

> I know it's kind of trivial, but it's also a trivial
> change in the test.   In this particular case, and certainly in hindsight,
> if the original test data added in bug 1402064 had the platform sections,
> this would have been caught.  What cant be caught with dummy data is a later
> change in AMO.

The test data *was* derived from AMO.  It was then modified to cover additional corner cases (eg, the platform: UNKNOWN bits).  It didn't include any addons that have separate per-platform files because I couldn't find any and was under the impression that that feature was going away.

I don't think testing the real platform values here is all that useful since we have to hard-code them, its a test that has to be backed up with manual testing and is not likely to every uncover a regression.

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260806/diff/1-2/

Comment 21

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

https://reviewboard.mozilla.org/r/260806/#review268140

Comment 22

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #19)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> > (In reply to Andrew Swan [:aswan] from comment #17)
> > > I saw that you made some test changes, but I don't think there was actually
> > > any expanded test coverage in that patch?
> > 
> > The test data has "all" for platform, so the platform specific check itself
> > is never tested.
> 
> The patches on bug 1478669 didn't actually exercise that code path, but I'll
> add something.

It did, but then it still passed when I put back the "all" part, I missed that.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6ec69871ae1
Fix platform matching in AMO metadata requests r=mixedpuppy

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/f6ec69871ae1

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

I'm not sure exactly who wants this or why, but ddurst mentioned it was wanted in 62.  Unfortunately he's on PTO until next Tuesday.  Perhaps this is already on relman's radar, if not then we can have ddurst complete the uplift request form when he's back...

Comment 26

[David Durst [:ddurst]]

Shane wanted it via bug 1478669 as part of getting data used for bug 1468680. I don't think we need this in 62.

Comment 27

[David Durst [:ddurst]]

Sorry, Add-ons doesn't want it for 62, but relman does (this was flagged in the regression triage on Wednesday). Sorry about that. I'll update it for beta uplift unless someone beats me to it.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Syncing addons is broken due to this. Andrew/David, can you please also request uplift into esr60?

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

Needed for addon sync to work correctly, let's uplift for beta 15.

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Some extensions are not properly synced

Fix Landed on Version:
63, also approved for uplift to 62

Risk to taking this patch (and alternatives if risky): 
The risk is low, the change is confined to the code that handles addon manager AMO metadata requests, and it only affects results that are already handled incorrectly.

String or UUID changes made by this patch: 
none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/83e81646f500

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

This feels like a bit of a stretch for an ESR IMO, but since we're early in the cycle and it includes automated tests, I'll approve for ESR 60.2.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/c290fd8d532b

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_AddonRepository.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=192443878&repo=mozilla-esr60

https://hg.mozilla.org/releases/mozilla-esr60/rev/55607870c37b

Comment 35

[Victor Carciu]

Can you please add some STRs for this bug?

Comment 36

[Andrew Swan [:aswan]]

1. Using a version of Firefox that does not have the fix for this bug landed, install an extension that only has platform-specific files (eg the one mentioned in comment 0)
2. Close the browser, check the files addons.json in the profile, the entry for the extension from step 1 should have null (?) as the value for the sourceURI property.
3. Using the same profile from step 1, run a recently Nightly that includes the fix for this bug
4. Either wait for 24 hours or open the browser console and run:
AddonManager.backgroundUpdateCheck();

5. Check addons.json again as in step 2, the entry should now include a string containing a URL for the sourceURI property.

Comment 37

[Andrew Swan [:aswan]]

Re esr, this is a conflict with bug 1455402.  I think I can come up with something that works though.

Comment 38

[Andrew Swan [:aswan]]

Attachment: Patch tweaked for esr60

MozReview-Commit-ID: EvjeXdCyLxi

Comment 39

[Mark Hammond [:markh] [:mhammond]]

(In reply to Andrew Swan [:aswan] from comment #36)
> 2. Close the browser, check the files addons.json in the profile, the entry
> for the extension from step 1 should have null (?) as the value for the
> sourceURI property.

FTR, I believe it will not have that property at all and will appear after the backgroundUpdateCheck() runs (but in general, either null or missing should be treated as the same "bad" case, and a string with a https:// url pointing at amo is the "good" case)

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/58fe6e169c8b

Comment 41

[Victor Carciu]

I verified the issue in nightly by following the steps from C:\Users\<user name>\AppData\Roaming\Mozilla\Extensions\{ec8030f7-c20a-464f-9b0e-13a3a9e97384}\ (I checked the addons.json file after 24 hours and I saw the correct sourceURI)

After that, I tried to verify the issue in ESR60 too , but without waiting 24 hrs and I ran following command in browser console, but I was not able to see the sourceURI after that.

ChromeUtils.import("resource://gre/modules/AddonManager.jsm");
AddonManagerPrivate.backgroundUpdateCheck()

Can you please let me know if this if we have an issue here?

Thanks,
Victor

Comment 42

[Victor Carciu]

(In reply to Victor Carciu from comment #41)
> I verified the issue in nightly by following the steps from C:\Users\<user
> name>\AppData\Roaming\Mozilla\Extensions\{ec8030f7-c20a-464f-9b0e-
> 13a3a9e97384}\ (I checked the addons.json file after 24 hours and I saw the
> correct sourceURI)
> 
> After that, I tried to verify the issue in ESR60 too , but without waiting
> 24 hrs and I ran following command in browser console, but I was not able to
> see the sourceURI after that.
> 
> ChromeUtils.import("resource://gre/modules/AddonManager.jsm");
> AddonManagerPrivate.backgroundUpdateCheck()
> 
> Can you please let me know if this if we have an issue here?
> 
> Thanks,
> Victor

Something was wrongly copy pasted in the text from above so I will add the first sentence corrected below : 
I verified the issue in nightly by following the steps from https://bugzilla.mozilla.org/show_bug.cgi?id=1476177#c36 (I checked the addons.json file after 24 hours and I saw thecorrect sourceURI)

Comment 43

[Victor Carciu]

Verified again using Firefox ESR 60.1.1esr(buildid 20180810210502), Firefox Beta 62.0b16 (buildid 20180809104529) and Firefox Nightly 63.0a1(buildid 20180814100100) in Win10x64 and this time everything worked as expected (sourceURI correctly displayed after running > ChromeUtils.import("resource://gre/modules/AddonManager.jsm");AddonManagerPrivate.backgroundUpdateCheck()in browser console)

I will attach a postfix screenshot.

Comment 44

[Victor Carciu]

Attachment: Postfix screenshot