Closed Bug 897735 Opened 7 years ago Closed 6 years ago

Support regular expression filters for metadata in extension blocks

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jorgev, Assigned: sachin)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [squeaky][engineering-friend])

Attachments

(1 file, 8 obsolete files)

We only support blocking extensions by ID at the moment, and that has forced us to write Add-ons Manager patches to deal with add-ons that are distributed with multiple IDs (like on bug 799266). We should add support for more advanced blocks for extensions.

What we want is filter by add-on name and add-on creator, using regular expressions. The current add-on ID filter already supports regular expressions, so there's probably some code there than can be reused. If more than one of the 3 filters is used, all 3 must match for an add-on to be blocked.

Here are examples of what I think these blocks should look like.

Only ID filter:

> <emItem blockID="XXX" id="/^pink@.*\.info$/">
>   <versionRange  minVersion="0" maxVersion="*" severity="3">
>   </versionRange>
> </emItem>


Only name filter:

> <emItem blockID="XXX" name="/^Mozilla Corp\.$/">
>   <versionRange  minVersion="0" maxVersion="*" severity="1">
>   </versionRange>
> </emItem>

All filters:

> <emItem blockID="XXX" id="/^pink@.*\.info$/ name="/^Mozilla Corp\.$/"
>         creator="/evild00d/">
>   <versionRange  minVersion="0" maxVersion="*" severity="1">
>   </versionRange>
> </emItem>

Since we already support regular expressions in the ID, I suppose the code already handles multiple matches, but it'd be good to verify this is the case.
This needs changes to the nsIBlocklistService interface. An add-on's ID is no longer sufficient to decide if it is blocklisted or not.

I will need to change everything that uses this component. I hope that is okay.
Flags: needinfo?
But passing the ID is sufficient information, no? On the implementation side you can get whatever data is necessary to determine if the add-on is blocklisted. So, I don't think it's necessary to change the interface.
Flags: needinfo?
Ah, sorry. :( I should have realized.
So I tried to use AddonManager API to retrieve the name and creator from the ID.
But the problem is that getAddonByID works asynchronously.
A call to getAddonBlocklistState here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#903 is where it is decided if an add-on is blocklisted.
But getAddonBlocklistState is part of nsIBlocklistService.
If we don't want to alter the interface, only at some point inside getAddonBlocklistState should we fetch the add-on's creator and name. But getAddonByID is asynchronous.
Since getAddonByID takes time, getBlocklistState proceeds to return, not waiting for the name and creator to be fetched.

I'm left to think that changing getAddonBlocklistState (to make it accept the name and creator as params) might be necessary. But this means changing nsIBlocklistService.

Or maybe we should cache the name and creator information in the object for use by getAddonBlocklistState.

What do you think?
Attached patch cache version (obsolete) — Splinter Review
The add-on is stored in _addonCache for use while checking.
I think we should also be able to block by updateURL, and probably homepage.
(In reply to Sachin Hosmani [:sachin] from comment #4)
> Or maybe we should cache the name and creator information in the object for
> use by getAddonBlocklistState.
> 
> What do you think?

Maybe we just need to cache the add-on ID and blocklist state in the object. I assume the blocklist service knows this information every time the blocklist is read and updated.

If we were forced to change the service API, it would probably make more sense to make it asynchronous too, but I don't think we need to for now.

(In reply to Kris Maglione [:kmag] from comment #6)
> I think we should also be able to block by updateURL, and probably homepage.

I agree with this. Let's include them on this bug.
Summary: Support regular expression filters for "name" and "creator" in extension blocks → Support regular expression filters for metadata in extension blocks
Attachment #783234 - Flags: review?(jorge)
Comment on attachment 783234 [details] [diff] [review]
cache version

We discussed the possible approaches for this and agreed that this was the least disruptive one. This is still missing Kris's suggested additions, and of course a review by someone who better understands this code :)
Attachment #783234 - Flags: review?(jorge) → review+
The idea of this patch is to cache (in the constructor of the blocklist service constructor) all the add-ons installed so that when _findMatchingAddonEntry is called with just the ID of the add-on, it can use the initially created cache to get the other attributes of the add-on (like creator, name etc.).

But this doesn't seem to work well either. The AddonManager uses the blocklist service on startup. What I noticed is, the cache isn't created yet when the AddonManager is using the blocklist service. This leads to incorrect blocklist states getting fetched. This is again probably because the cache is loaded asynchronously.

Jorge, the other solution you told me on IRC and in comment 7 also leads to the same issue since that also involves creating a map using AddonManager's asynchronous API.

Is changing the blocklist service into an asynchronous API the only way?
Attachment #783234 - Attachment is obsolete: true
Whiteboard: [squeaky] → [squeaky][engineering-friend]
(In reply to Sachin Hosmani [:sachin] from comment #9)
> Jorge, the other solution you told me on IRC and in comment 7 also leads to
> the same issue since that also involves creating a map using AddonManager's
> asynchronous API.

My suggestion from comment #7 is that we populate a local cache on the blocklist service when it is run, and all we need to store are the add-on ID and its blocklist status. I don't really understand why this can't be accomplished without additional calls to the Add-ons Manager.
Additional calls to the Add-ons Manager aren't necessary in both my patch and what you suggest. What we need is just an initial call as you say. But this initial call is itself causing problems.

I think there are 2 problems in what you suggest:
1. Some add-ons may be blocked by a combination of attributes that excludes add-on ID. In this case, we won't be able to store the add-on's ID in the cache. The workaround you had suggested where we search for installed add-ons that match the blocklist attributes and find the IDs, will need to use the Add-on Manager API.
2. If a to-be-blocked add-on is installed after the blocklist is read, the add-on's ID will not be ready in the cache if the blocklist entry didn't contain the add-on's ID as an attribute of an entry. (This problem exists in my patch as well.)

In the patch, I build an add-on cache containing installed add-ons in the constructor of the service. (This can't be done only when the blocklist file is updated, because a browser restart between blocklist updates will empty the cache.) This action uses the Add-on Manager API which is causing problems.

Hope I'm not missing something.
(In reply to Sachin Hosmani [:sachin] from comment #11)
> What we need is just an initial call as you say. But
> this initial call is itself causing problems.

Which initial call? When is this needed?

> 1. Some add-ons may be blocked by a combination of attributes that excludes
> add-on ID. In this case, we won't be able to store the add-on's ID in the
> cache.

All I'm saying is that we should have a mapping that has <add-on ID, blocklist status>. That's all we need to know in order for the current API to continue working. This doesn't need to be related to any other data structures we use internally.

> 2. If a to-be-blocked add-on is installed after the blocklist is read, the
> add-on's ID will not be ready in the cache if the blocklist entry didn't
> contain the add-on's ID as an attribute of an entry. (This problem exists in
> my patch as well.)

Then let's just trigger a blocklist read when an add-on is installed, regardless of whether we use your approach or mine. Clearly the rules are much more complex now and going through an ID list won't suffice.
(In reply to Jorge Villalobos [:jorgev] from comment #12)
> (In reply to Sachin Hosmani [:sachin] from comment #11)
> > What we need is just an initial call as you say. But
> > this initial call is itself causing problems.
> 
> Which initial call? When is this needed?
I was referring to the AddonManager calls that will get the add-on's ID from other properties in your approach (to build the map) and vice versa in mine, each of which are necessary in their respective cases.

> > 1. Some add-ons may be blocked by a combination of attributes that excludes
> > add-on ID. In this case, we won't be able to store the add-on's ID in the
> > cache.
> 
> All I'm saying is that we should have a mapping that has <add-on ID,
> blocklist status>. That's all we need to know in order for the current API
> to continue working. This doesn't need to be related to any other data
> structures we use internally.

I assume you are suggesting we build this map when the blocklist service parses the blocklist file. So consider a blocklist entry with out an add-on's ID specified. How do we add this add-on to the map? We could fetch the add-on's ID from the AddonManager but that will be an asynchronous call.

> > 2. If a to-be-blocked add-on is installed after the blocklist is read, the
> > add-on's ID will not be ready in the cache if the blocklist entry didn't
> > contain the add-on's ID as an attribute of an entry. (This problem exists in
> > my patch as well.)
> 
> Then let's just trigger a blocklist read when an add-on is installed,
> regardless of whether we use your approach or mine. Clearly the rules are
> much more complex now and going through an ID list won't suffice.

You mean to say that an add-on ID can't remain the sole attribute in the blocklist file that decides an add-on's blocklist state, right?
Is there a particular reason to work so hard to keep the existing API? Seems like this would be far easier to just change it so you pass the addon object with all its properties there for synchronous access.
(In reply to Sachin Hosmani [:sachin] from comment #13)
> > All I'm saying is that we should have a mapping that has <add-on ID,
> > blocklist status>. That's all we need to know in order for the current API
> > to continue working. This doesn't need to be related to any other data
> > structures we use internally.
> 
> I assume you are suggesting we build this map when the blocklist service
> parses the blocklist file. So consider a blocklist entry with out an
> add-on's ID specified. How do we add this add-on to the map? We could fetch
> the add-on's ID from the AddonManager but that will be an asynchronous call.

Okay, but that would happen while the blocklist is being read, which is not a problem as far as I know. The sync/async problem is when someone is calling the blocklist service and asking if a particular add-on is blocklisted.

> > > 2. If a to-be-blocked add-on is installed after the blocklist is read, the
> > > add-on's ID will not be ready in the cache if the blocklist entry didn't
> > > contain the add-on's ID as an attribute of an entry. (This problem exists in
> > > my patch as well.)
> > 
> > Then let's just trigger a blocklist read when an add-on is installed,
> > regardless of whether we use your approach or mine. Clearly the rules are
> > much more complex now and going through an ID list won't suffice.
> 
> You mean to say that an add-on ID can't remain the sole attribute in the
> blocklist file that decides an add-on's blocklist state, right?

Well, not really. I mean, that last statement is true, we will no longer have IDs for all add-on blocks. But what I was trying to say is that the internal list of IDs in the blocklist service will not suffice anymore. So, when a new add-on is installed, we will need more in order to determine its blocklist state.

(In reply to Dave Townsend (:Mossop) from comment #14)
> Is there a particular reason to work so hard to keep the existing API? Seems
> like this would be far easier to just change it so you pass the addon object
> with all its properties there for synchronous access.

I thought it'd be better for compatibility purposes, since it seemed to be an unnecessary change. But if things are getting so complicated (and I guess they are), we should consider this approach. Passing the addon object would be cleaner than passing individual attributes, for sure. If all the callers have that object available, it shouldn't be a problem to switch to that (other than potential add-on compat).
(In reply to Jorge Villalobos [:jorgev] from comment #15)

> Okay, but that would happen while the blocklist is being read, which is not
> a problem as far as I know.

Um, I think it is a problem. A line of async code in the midst of synchronously executing code seems wrong to me, since the data that is retrieved from the async call is needed.
(In reply to Jorge Villalobos [:jorgev] from comment #15)
> I thought it'd be better for compatibility purposes, since it seemed to be
> an unnecessary change. But if things are getting so complicated (and I guess
> they are), we should consider this approach. Passing the addon object would
> be cleaner than passing individual attributes, for sure. If all the callers
> have that object available, it shouldn't be a problem to switch to that
> (other than potential add-on compat).

I see a few add-ons on AMO doing the right thing and checking the blocklistState property to check it, none using nsIBlocklistService though (aside from the constants there). Given that we already do additional blocklist checks outside of nsIBlocklistService then if they're using that they're not getting the correct answer so I don't think we need to avoid breaking this.
- Shall I remove the hard-coded block entries added in bug 799266?

- Since updateURL isn't a property of AddonWrapper I'm not sure of how to get that property.
While we could pass AddonInternal objects to blocklist service methods, the blocklist service itself uses AddonManager to iterate through all the installed add-ons when the blocklist gets updated and AddonManager returns AddonWrapper objects. (http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/nsBlocklistService.js#896)
Flags: needinfo?(jorge)
(In reply to Sachin Hosmani [:sachin] from comment #18)
> - Shall I remove the hard-coded block entries added in bug 799266?

No, I think we should leave them in, since we want to maximize their coverage, and only new users will benefit from these changes.
Flags: needinfo?(jorge)
Could you please suggest a workaround for the other question in comment 18?
Flags: needinfo?(dtownsend+bugmail)
(In reply to Sachin Hosmani [:sachin] from comment #20)
> Could you please suggest a workaround for the other question in comment 18?

I don't see a reason why we couldn't expose updateURL. The question becomes whether we expose exactly what is in the install.rdf or we first do the substitutions from http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#5603. I'm leaning towards the latter but I'm not sure what our needs are for blocklisting and this is Blair's call really.
Flags: needinfo?(dtownsend+bugmail) → needinfo?(bmcbride)
Is the the purpose of blocklist API to find blocklist information of only installed add-ons or also other add-ons (ones not installed)? I mean to ask if it is used to simply read the contents of the blocklist file. This fix won't work for that kind of usage since we can't get an add-on object for add-ons that aren't installed.

In test_bug393285.js, I think the API is used on add-ons that aren't installed.
What do I do about such tests?
Some tests like in comment 22 failed.
Attachment #788499 - Attachment is obsolete: true
(In reply to Sachin Hosmani [:sachin] from comment #22)
> Is the the purpose of blocklist API to find blocklist information of only
> installed add-ons or also other add-ons (ones not installed)? I mean to ask
> if it is used to simply read the contents of the blocklist file. This fix
> won't work for that kind of usage since we can't get an add-on object for
> add-ons that aren't installed.

As long as it works for installed or about to be installed add-ons then I think that that is all that matters.

> In test_bug393285.js, I think the API is used on add-ons that aren't
> installed.
> What do I do about such tests?

Just create mock Addon objects for those
(In reply to Dave Townsend (:Mossop) from comment #21)
> (In reply to Sachin Hosmani [:sachin] from comment #20)
> > Could you please suggest a workaround for the other question in comment 18?
> 
> I don't see a reason why we couldn't expose updateURL. The question becomes
> whether we expose exactly what is in the install.rdf or we first do the
> substitutions

Eh, simple = good. Lets just go with the value from install.rdf.

Just exposing the raw URL from install.rdf is far simpler, and that's all we need for this really. And other consumer is probably only going to want the domain. There's also the question of if we did do the substitutions, what would UPDATE_TYPE be substituted for?
Flags: needinfo?(bmcbride)
Comment on attachment 808207 [details] [diff] [review]
supports attributes except updateURL

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

*Much* prefer this approach. 

I've previously stated that any change that requires the API to become async just has to wait for the blocklist to be rewritten (bug 782261) - because switching to async APIs is just too big of a change (and too regression prone) for a codebase we want to throw away soon anyway.
Attachment #808207 - Flags: feedback+
Attached patch supports all attributes (obsolete) — Splinter Review
I've exposed updateURL as it is in install.rdf.
I also fixed the old tests that are affected by the change in API.
Attachment #808207 - Attachment is obsolete: true
Attachment #819814 - Flags: review?(bmcbride)
Comment on attachment 819814 [details] [diff] [review]
supports all attributes

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

Looks good :)

One last remaining issue: There are a few other areas of the tree in both /toolkit/ and /browser/ that access these APIs - need to update those too so we don't break things. eg, SocialService.jsm, aboutDialog.js, etc.

Also, we'll need a bug filed to ensure someone checks Thunderbird and Seamonkey for usage of these APIs.

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +766,4 @@
>        versions: [],
>        prefs: [],
> +      blockID: null,
> +      attributes : {}

Since this used to hold data we loop over, it should be a Map.

@@ +767,5 @@
>        prefs: [],
> +      blockID: null,
> +      attributes : {}
> +      // Atleast one of these must get added to attributes:
> +      // id, creator, name, updateURL, homepageURL

Nit: Reduce maintenance, don't explicitly list the properties - just mention EXTENSION_BLOCK_FILTERS.

@@ +772,4 @@
>      };
>  
> +    // Add-on attributes don't start with '/', so any of those
> +    // starting with '/' must be a regex.

Nit: This is only technically true for IDs. So say here that any filters that start with '/' are just interpreted as a regex. And if we need to match an attribute value starting with '/', the filter needs to do so via a regex.

@@ +772,5 @@
>      };
>  
> +    // Add-on attributes don't start with '/', so any of those
> +    // starting with '/' must be a regex.
> +    function RegExpCheck(attr) {

Nit: Code style. Functions don't start with a capital letter.
Attachment #819814 - Flags: review?(bmcbride) → review-
Sachin, will you be able to continue working on this?
Flags: needinfo?(sachinhosmani2)
Blocks: 943194
Yes I will. I can get to it in about a week.
I got busy because of exams and college work.
Flags: needinfo?(sachinhosmani2)
In here: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/social/SocialService.jsm#592 ,
a blocklist state check is done by ID read from the manifest. But that is synchronously executing code. Since there is no callback passed there, we can't get an add-on object with the Add-on Manager API.

I'm thinking we could just create dummy add-on objects with just whatever we need that is in the manifest : [https://developer.mozilla.org/en-US/docs/Social_API/Manifest] and pass it to the blocklist service.
We can get everything but updateURL from the manifest as I see it.

Will this be ok?
Flags: needinfo?(bmcbride)
I'm not familiar with SocialService.jsm, so better to see what Shane thinks.

Shane: nsIlocklistService.getAddonBlocklistState() is changing from accepting an addon ID and version string to taking an Addon object. Sachin mentions in comment 31 that this might be a bit awkward for the case he identified. Thoughts?
Flags: needinfo?(scaraveo)
(In reply to Blair McBride [:Unfocused] from comment #32)
> I'm not familiar with SocialService.jsm, so better to see what Shane thinks.
> 
> Shane: nsIlocklistService.getAddonBlocklistState() is changing from
> accepting an addon ID and version string to taking an Addon object. Sachin
> mentions in comment 31 that this might be a bit awkward for the case he
> identified. Thoughts?

There are a couple places that need to be updated.  IIUC we just need to create an instance of AddonWrapper (Also in SocialService.jsm) passing the manifest as the argument.

.getAddonBlocklistState(new AddonWrapper(data), ...)

Am I reading correctly that the api is remaining synchronous?  If not, there is a bit more effort to make this work.
Flags: needinfo?(scaraveo)
Attached patch socialapi changes (obsolete) — Splinter Review
I think these changes would do it for socialapi, I haven't tested this though.
Attached patch socialapi changes (obsolete) — Splinter Review
sigh...missed a file
Attachment #8349565 - Attachment is obsolete: true
Attached patch blocklist-regexp.patch (obsolete) — Splinter Review
Made those changes.
Attachment #819814 - Attachment is obsolete: true
Attachment #8349982 - Flags: review?(bmcbride)
Blocks: 936214
Blocks: 949570
Flags: needinfo?(bmcbride)
Comment on attachment 8349982 [details] [diff] [review]
blocklist-regexp.patch

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

Good to go!

I forget, do you have access to land this yourself?

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +766,4 @@
>        versions: [],
>        prefs: [],
> +      blockID: null,
> +      attributes : new Map()

Nit: No space before the colon.
Attachment #8349982 - Flags: review?(bmcbride) → review+
Attachment #8349568 - Attachment is obsolete: true
No I don't have access to land this.
Attached patch blocklist-regexp.patch (obsolete) — Splinter Review
Attachment #8349982 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/5b19ca07781b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [squeaky][engineering-friend] → [squeaky][engineering-friend][fixed-in-fx-team]
\o/ Broke the social tests! None of which this test actually touched... *sigh*

Any idea what's up, Sachin? Or would would you like Shane to have a look? (Cos I have no idea)
Status: NEW → ASSIGNED
Flags: needinfo?(sachinhosmani2)
Whiteboard: [squeaky][engineering-friend][fixed-in-fx-team] → [squeaky][engineering-friend]
No, I don't understand what is happening either. Shane, can you have a look?
Flags: needinfo?(sachinhosmani2) → needinfo?(mixedpuppy)
(In reply to Sachin Hosmani [:sachin] from comment #43)
> No, I don't understand what is happening either. Shane, can you have a look?

I'll do a build with this patch and see what's up.
patch with socialapi fixes.  try at:

https://tbpl.mozilla.org/?tree=Try&rev=d8df60540bdf
Attachment #8356014 - Attachment is obsolete: true
Flags: needinfo?(mixedpuppy)
try looks ok, imo this can be checked in again.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/933bf791b773
Keywords: checkin-needed
Whiteboard: [squeaky][engineering-friend] → [squeaky][engineering-friend][fixed-in-fx-team]
Backed out for completely destroying Windows mochitests.
https://hg.mozilla.org/integration/fx-team/rev/716a6ec17cec

https://tbpl.mozilla.org/php/getParsedLog.php?id=32978161&tree=Fx-Team

This looks relevant:
07:54:32     INFO -  *** ERROR addons.xpi: Unable to read add-on manifest from c:\users\cltbld\appdata\local\temp\tmpakyznu\extensions\staged\workerbootstrap-test@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6209"  data: no]
07:54:32     INFO -  *** ERROR addons.xpi: Unable to read add-on manifest from c:\users\cltbld\appdata\local\temp\tmpakyznu\extensions\staged\worker-test@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6209"  data: no]
07:54:32     INFO -  *** ERROR addons.xpi: Unable to read add-on manifest from c:\users\cltbld\appdata\local\temp\tmpakyznu\extensions\staged\special-powers@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6209"  data: no]
07:54:32     INFO -  *** ERROR addons.xpi: Unable to read add-on manifest from c:\users\cltbld\appdata\local\temp\tmpakyznu\extensions\staged\mochikit@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6209"  data: no]
07:54:32     INFO -  *** ERROR addons.xpi: Unable to read add-on manifest from c:\users\cltbld\appdata\local\temp\tmpakyznu\extensions\staged\indexedDB-test@mozilla.org: [Exception... "Not enough arguments [nsIBlocklistService.getAddonBlocklistState]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: AddonInternal.prototype.blocklistState :: line 6209"  data: no]
Flags: in-testsuite+
Whiteboard: [squeaky][engineering-friend][fixed-in-fx-team] → [squeaky][engineering-friend]
Please make sure this has a green Try run before requesting checkin again.
Hmm, looks like it was needs-clobber bustage. Re-landed with a CLOBBER touch. Sorry for the churn.
https://hg.mozilla.org/integration/fx-team/rev/c8e2cb87f6c1
I filed bug 959653 for the needs-clobber.
You did not update nsIBlocklistService's IID when changing the interface.  That's what caused the build failures.  You need to push a followup to do that.
The issue was a missing IID bump to nsIBlocklistService.
https://hg.mozilla.org/integration/fx-team/rev/8c0180723f67
I'm still seeing this, even after clobber building fx-team tip a couple times in the last hour.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #53)
> The issue was a missing IID bump to nsIBlocklistService.
> https://hg.mozilla.org/integration/fx-team/rev/8c0180723f67

Aaaand that accidentally un-did all other changes in that file too. Pushed a fix for that:
https://hg.mozilla.org/integration/fx-team/rev/869ff4bc5354
(In reply to Brandon Benvie [:benvie] from comment #54)
> I'm still seeing this, even after clobber building fx-team tip a couple
> times in the last hour.

A build error, or general brokenness that may be accounted for by comment 55?
(In reply to Blair McBride [:Unfocused] from comment #56)
> (In reply to Brandon Benvie [:benvie] from comment #54)
> > I'm still seeing this, even after clobber building fx-team tip a couple
> > times in the last hour.
> 
> A build error, or general brokenness that may be accounted for by comment 55?

Looks like the patch in comment 55 will fix this.
*sigh* Can I just quit while I'm behind?
Blocks: 977375
Blocks: 1006794
Blocks: 1048672
You need to log in before you can comment on or make changes to this bug.