Last Comment Bug 907893 - nsIBrowserSearchEngine interface on FF24b needs a uuid bump
: nsIBrowserSearchEngine interface on FF24b needs a uuid bump
Status: RESOLVED FIXED
[qa-]
: addon-compat
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: 24 Branch
: All All
: -- normal (vote)
: Firefox 26
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
https://hg.mozilla.org/releases/mozil...
Depends on:
Blocks: 493051
  Show dependency treegraph
 
Reported: 2013-08-21 12:55 PDT by Alex Vincent [:WeirdAl]
Modified: 2013-10-16 14:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
fixed


Attachments
uuid bump (933 bytes, patch)
2013-08-22 14:28 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Review

Description Alex Vincent [:WeirdAl] 2013-08-21 12:55:52 PDT
On FF24b, there was a change to nsIBrowserSearchService's methods (addEngine in particular) that didn't get a corresponding uuid change.  The latest uuid change to that interface appears to predate the method change.

I haven't checked FF25 aurora yet, or trunk.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-22 14:28:50 PDT
Created attachment 794266 [details] [diff] [review]
uuid bump

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 493051
User impact if declined: potential for crashing binary addons
Testing completed (on m-c, etc.): N/A
Risk to taking this patch (and alternatives if risky): compatibility break for addons on beta that have already been compiled. Alternative for beta is to not take the change, in which case addons calling this method may crash.
String or IDL/UUID changes made by this patch: yes, that's the intent.
Comment 2 bhavana bajaj [:bajaj] 2013-08-22 19:44:27 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> Created attachment 794266 [details] [diff] [review]
> uuid bump
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 493051
> User impact if declined: potential for crashing binary addons
> Testing completed (on m-c, etc.): N/A
> Risk to taking this patch (and alternatives if risky): compatibility break
> for addons on beta that have already been compiled. Alternative for beta is
> to not take the change, in which case addons calling this method may crash.
> String or IDL/UUID changes made by this patch: yes, that's the intent.

Do we know of any addon's/plugins that using this interface and may be impacted ? In the past we have gone with option of actually not bumping the IID post Beta 1 and since the addon testing is already completed by this time and instead did some outreach letting them know about the issue.But, :Gavin points out the risk of a potential crash in this situation :( Is taking the patch the lowest risk option here ?
Comment 3 Jorge Villalobos [:jorgev] 2013-08-23 07:41:55 PDT
The potential breakage here would happen with binary add-ons, and we never know for sure which interfaces they are using, since they are mostly hosted outside of AMO. Given that many alter search settings it's very possible that some would be affected...

I don't think there's a good solution here, but I would favor doing the uuid bump for the sake of correctness.
Comment 4 bhavana bajaj [:bajaj] 2013-08-23 09:53:15 PDT
(In reply to Jorge Villalobos [:jorgev] from comment #3)
> The potential breakage here would happen with binary add-ons, and we never
> know for sure which interfaces they are using, since they are mostly hosted
> outside of AMO. Given that many alter search settings it's very possible
> that some would be affected...
> 
> I don't think there's a good solution here, but I would favor doing the uuid
> bump for the sake of correctness.

Thanks Jorgev, Approving the patch and I will work with you on proactively trying to contact popular addon's like we've done in the past.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-24 08:53:20 PDT
https://hg.mozilla.org/integration/fx-team/rev/6159d3584b37
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-08-24 11:54:21 PDT
Apparently landing this required a clobber on Windows.
Comment 7 Tim Taubert [:ttaubert] 2013-08-25 06:12:51 PDT
https://hg.mozilla.org/mozilla-central/rev/6159d3584b37
Comment 9 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-09-12 16:34:25 PDT
Assuming no QA needed here. Naturally we'll keep an eye out for any addon-compat issues as is our normal process. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.

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