Last Comment Bug 725083 - Handle undefined sourceURI when installing add-ons from search result
: Handle undefined sourceURI when installing add-ons from search result
Status: VERIFIED FIXED
[inbound]
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Gregory Szorc [:gps]
:
:
Mentors:
Depends on:
Blocks: 724661
  Show dependency treegraph
 
Reported: 2012-02-07 13:59 PST by Gregory Szorc [:gps]
Modified: 2013-01-27 22:56 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
Handle undefined sourceURI in search results, v1 (5.25 KB, patch)
2012-02-07 13:59 PST, Gregory Szorc [:gps]
no flags Details | Diff | Splinter Review
Handle undefined sourceURI in search results, v2 (6.30 KB, patch)
2012-02-07 14:02 PST, Gregory Szorc [:gps]
rnewman: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Gregory Szorc [:gps] 2012-02-07 13:59:57 PST
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.
Comment 1 Gregory Szorc [:gps] 2012-02-07 14:02:56 PST
Created attachment 595167 [details] [diff] [review]
Handle undefined sourceURI in search results, v2

Twas missing added test XML file.
Comment 2 Richard Newman [:rnewman] 2012-02-07 14:09:05 PST
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 4 Gregory Szorc [:gps] 2012-02-07 14:21:36 PST
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] 2012-02-08 09:02:48 PST
https://hg.mozilla.org/mozilla-central/rev/c9bbbe2acb37
Comment 6 Alex Keybl [:akeybl] 2012-02-09 13:54:21 PST
(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] 2012-02-09 22:36:15 PST
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] 2012-02-10 13:10:35 PST
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] 2012-02-13 09:22:55 PST
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.

Note You need to log in before you can comment on or make changes to this bug.