Handle undefined sourceURI when installing add-ons from search result

VERIFIED FIXED in Firefox 11

Status

Cloud Services
Firefox Sync: Backend
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(firefox11+ fixed, firefox12+ fixed)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 595164 [details] [diff] [review]
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.
Attachment #595164 - Flags: review?(rnewman)
(Assignee)

Comment 1

5 years ago
Created attachment 595167 [details] [diff] [review]
Handle undefined sourceURI in search results, v2

Twas missing added test XML file.
Assignee: nobody → gps
Attachment #595164 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #595164 - Flags: review?(rnewman)
Attachment #595167 - Flags: review?(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.

^-^
Attachment #595167 - Flags: review?(rnewman) → review+
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9bbbe2acb37
status-firefox11: --- → affected
status-firefox12: --- → affected
tracking-firefox11: --- → ?
tracking-firefox12: --- → ?
Whiteboard: [inbound]
(Assignee)

Comment 4

5 years ago
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.
Attachment #595167 - Flags: approval-mozilla-beta?
Attachment #595167 - Flags: approval-mozilla-aurora?

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c9bbbe2acb37
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Comment 6

5 years ago
(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.
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.
Status: RESOLVED → VERIFIED

Comment 8

5 years ago
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.
Attachment #595167 - Flags: approval-mozilla-beta?
Attachment #595167 - Flags: approval-mozilla-beta+
Attachment #595167 - Flags: approval-mozilla-aurora?
Attachment #595167 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 9

5 years ago
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.
status-firefox12: affected → fixed

Updated

5 years ago
tracking-firefox11: ? → +
tracking-firefox12: ? → +
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/0ad647bce7b8
status-firefox11: affected → fixed
You need to log in before you can comment on or make changes to this bug.