725083 - Handle undefined sourceURI when installing add-ons from search result

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Handle undefined sourceURI in search results, v1

This bug was discovered in bug 724661. When installing add-ons from search results, it is possible that sourceURI was undefined on the add-on object. There were 2 issues:

1) This was throwing an uncaught exception
2) The completion callback would never get called because it was waiting for all of the search results to install. Add-ons without a sourceURI would never install, so we'd wait forever.

The attached patch handles undefined sourceURI properly. I will request beta uplift of the (eventual) patch and will thus probably land this bug directly in inbound to speed uplift.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Handle undefined sourceURI in search results, v2

Twas missing added test XML file.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 595167 [details] [diff] [review]
Handle undefined sourceURI in search results, v2

Review of attachment 595167 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/engines/addons.js
@@ +966,5 @@
>          // complicated at the time the functionality was implemented.
>          for each (let addon in addons) {
> +          // sourceURI should be defined. But, since the input comes from an
> +          // external source and it isn't validated in AddonRepository, we
> +          // can't rely on it. We simply skip add-ons without a sourceURI.

"should" is a little ambiguous: could be read as "you should set sourceURI".

How about:

// External input isn't validated in AddonRepository, so here we skip
// add-ons without a sourceURI.

@@ +1002,5 @@
>  
> +        expectedInstallCount = toInstall.length;
> +
> +        // If no expected installs, invoke callback ourselves with empty result
> +        // set.

Add 1 to x.

^-^

Comment 3

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bbbe2acb37

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 595167 [details] [diff] [review]
Handle undefined sourceURI in search results, v2

[Approval Request Comment]
Regression caused by (bug #): 534956 (bug in add-on sync first drop)
User impact if declined: Sync could silently stop working. This would occur every time Sync runs and you would never finish a sync. Users would probably see an error bar after 14 days.
Testing completed (on m-c, etc.): Just landed on inbound.
Risk to taking this patch (and alternatives if risky): The patched code is almost certainly no less risky than add-on sync in its current state.
String changes made by this patch: None

This should probably bake for a few days on Nightly. And, I would like confirmation from Andreas on tomorrow's Nightly that this works around the reported issue.

Comment 5

[Ed Morley [:emorley]]

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

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Gregory Szorc [:gps] from comment #4)
> This should probably bake for a few days on Nightly. And, I would like
> confirmation from Andreas on tomorrow's Nightly that this works around the
> reported issue.

Holding in the queue to hear back about testing.

Comment 7

[Andreas Wagner [:TheOne] [use NI]]

As per bug 724661#c17 the patch is working fine. I can't interpret the log, so I don't know details. Add-on syncing might still fail but it won't stop the whole sync process.

I think Gregory can add more details if needed.

Thank you all for fixing this so quickly.

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 595167 [details] [diff] [review]
Handle undefined sourceURI in search results, v2

[Triage Comment]
Thanks Andreas - approving for Aurora 12 and Beta 11 based upon your testing.

Comment 9

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9d47691b06d9

I'm holding off pushing to Beta because bug 708134 was overlooked for 11 approval and the patch application gets messy. I'd prefer to apply that patch then this one so things apply cleanly.

Comment 10

[Gregory Szorc [:gps]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0ad647bce7b8