Closed Bug 1476177 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 62+ verified
firefox61 --- wontfix
firefox62 + verified
firefox63 + verified

People

(Reporter: markh, Assigned: aswan)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files)

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+
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.
Hey Andrew and Kris, any thoughts on this? It would be nice to fix addon sync as soon as we can.
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(aswan)
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.
Andrew, does anything about comment 4 ring a bell?
Flags: needinfo?(kmaglione+bmo) → needinfo?(awilliamson)
(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.
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.
That add-on is listed as explicitly supporting 3 separate platforms rather than just supporting all platforms.
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?
(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.
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.
Flags: needinfo?(awilliamson)
(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.
Assignee: nobody → aswan
Flags: needinfo?(aswan)
Priority: -- → P1
Duplicate of this bug: 1478669
> 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.
Attachment #8996753 - Flags: review?(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.
Attachment #8996753 - Flags: review?(mixedpuppy)
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)
(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.
(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 on attachment 8996753 [details]
Bug 1476177 Fix platform matching in AMO metadata requests

https://reviewboard.mozilla.org/r/260806/#review268140
Attachment #8996753 - Flags: review?(mixedpuppy) → review+
(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.
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6ec69871ae1
Fix platform matching in AMO metadata requests r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/f6ec69871ae1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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...
Flags: needinfo?(ddurst)
Attachment #8996753 - Flags: approval-mozilla-beta?
Shane wanted it via bug 1478669 as part of getting data used for bug 1468680. I don't think we need this in 62.
Flags: needinfo?(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.
Syncing addons is broken due to this. Andrew/David, can you please also request uplift into esr60?
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.
Attachment #8996753 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8996753 - Flags: approval-mozilla-esr60?
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.
Attachment #8996753 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
See Also: → 1481355
Can you please add some STRs for this bug?
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.
Flags: needinfo?(aswan)
Re esr, this is a conflict with bug 1455402.  I think I can come up with something that works though.
MozReview-Commit-ID: EvjeXdCyLxi
(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)
Flags: qe-verify+
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
Flags: needinfo?(aswan)
(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)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(aswan)
Attached image Postfix screenshot
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.