811193 - nsIUrlListManager.safeLookup not updated with bug 775796

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Hector Zhao [:hectorz]]

nsIUrlListManager.safeLookup is said to be the exception free wrapper[1] for nsIUrlClassifierDBService.lookup, but it was not changed with bug 775796 where the first argument to lookup switched from string to principal, and stops working since fx 17.

[1]: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#309

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Patch

Comment 2

[Hector Zhao [:hectorz]]

Comment on attachment 681049 [details] [diff] [review]
Patch

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

I'm not experienced enough to do a formal review, but with the changes in (https://hg.mozilla.org/mozilla-central/diff/88af948f119e/toolkit/components/url-classifier/nsIUrlClassifierDBService.idl) as a reference, this patch looks good to me.

Maybe a testcase should be included to make sure this doesn't happen again?

Comment 3

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab84754b536

Comment 4

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8ab84754b536

Comment 5

[Mark Banner (:standard8)]

Bug 775796 may provide some hints

Comment 6

[Mark Banner (:standard8)]

Opps sorry commented on wrong bug.

Comment 7

[Hector Zhao [:hectorz]]

Not really sure, but I remember once being told these flags and keywords should be set.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 681049 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 775796
User impact if declined: developers will no longer be able to use nsIUrlListManager.safeLookup
Risk to taking this patch (and alternatives if risky): there is only an UUID change, this is the only risk (there is no other code change). We might break addon compat but I believe we are soon enough in the m-a and m-b cycle that we could land this quite safely.
String or UUID changes made by this patch: yes, nsIUrlListManager

Comment 9

[Mounir Lamouri (:mounir)]

Note that we might think to land this in ESR (and Firefox 17?) if addons happen to be broken because of this.

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Mounir Lamouri (:mounir) from comment #8)
> [Approval Request Comment]
> User impact if declined: developers will no longer be able to use
> nsIUrlListManager.safeLookup

This appears to be a regression in FF17. Do we know of any add-ons broken by bug 775796? We need to be able to weigh maintaining the status quo for another 6 weeks versus addon compatibility impact.

> String or UUID changes made by this patch: yes, nsIUrlListManager

Including Jorge to help make the call on addon compatibility impact to beta here.

We'll approve for Aurora in the meantime.

Comment 11

[Jorge Villalobos [:jorgev] (he/him)]

I don't see any add-ons using safeLookup. Changing the interface now sounds okay to me for beta, but there is no urgency to push this through for add-ons.

I do see a couple of add-ons using functions that changed on bug 775796, which wasn't flagged addon-compat :\. However, they are very few and I can contact the developers directly.

Comment 12

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/271ed0c9bc96

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 681049 [details] [diff] [review]
Patch

Given Jorge's comment, let's leave this as is for FF18 (same as FF17).