1157490 - Immediately notify when a category entry is added/removed

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I'm writing some code where I register a content policy and do a load. Right now, the content policy doesn't affect the load because content policy uses an nsCategoryCache, which relies on an "xpcom-category-entry-added" notification. However, we fire that notification off an event to ensure it runs on the main thread.

If we're on the main thread already, I don't see why we couldn't fire the notification immediately. I could imagine this breaking stuff, but I think we should try it. Not too many add-ons appear to rely on this notification.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7aef746dc206

Comment 1

[Benjamin Smedberg]

This has broken stuff in the past, including many tests. Please tread carefully.

Comment 2

[Nathan Froyd [:froydnj]]

Try looks amazingly green for something that has broken things in the past.  Do you recall bug numbers or similar where folks have tried this?

Comment 3

[Benjamin Smedberg]

Was trolling bugzilla history and I can't find it. It was explicitly asynchronous all the way back to bug 246085. IIRC it was something to do with infinite recursion.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #0)
> I'm writing some code where I register a content policy and do a load. Right
> now, the content policy doesn't affect the load because content policy uses
> an nsCategoryCache, which relies on an "xpcom-category-entry-added"
> notification. However, we fire that notification off an event to ensure it
> runs on the main thread.

Can you point to patches or bugs where you're doing this?  I'm curious why the content policy registration has to happen in the same event loop tick as the (start of?) the load.

It looks like AdBlockPlus uses this notification...for something.  Can you try firing up a test session with ABP installed and browsing around to see if anything particularly bad happens, too?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> It looks like AdBlockPlus uses this notification...for something.  Can you
> try firing up a test session with ABP installed and browsing around to see
> if anything particularly bad happens, too?

With this patch applied, of course...

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8596228 [details] [diff] [review]
patch

Canceling review while we get this worked out.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I just debugged one of the the xpcshell failures and it's actually pretty serious. It may be the problem Benjamin was thinking of.

The test has a manifest that looks like this:
component 62e221d3-68c3-4e1a-8943-a27beb5005fe nsDummyObserver.js
contract @mozilla.org/places/test/dummy-observer;1 62e221d3-68c3-4e1a-8943-a27beb5005fe
category bookmark-observers nsDummyObserver @mozilla.org/places/test/dummy-observer;1

For some reason, the manifest parser processes category directives before contract directives, regardless of the order in which they appear in the file. When the category is added, my patch causes us to immediately run the code at [1], which looks up the dummy-observer contract ID. It doesn't exist yet, so we fail.

Perhaps it would be possible to change the order in which we process manifest directives, but that will probably just cause more trouble.

The reason I want this is for bug 1157561. It's an API for add-ons, and it would be really nice if things happen right away since that's how the Chrome version works. But maybe I'll just give up for now.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCategoryCache.cpp?rev=6fdb8eb0faac#138