368223 - nsIChromeRegistrySea.installPackage should respect xpcNativeWrappers flag

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[Wladimir Palant]

This bug affects Adblock Plus extension because the recent versions will break when running without XPCNativeWrappers (better that than opening a security hole). The issue looks like this:

1. User installs Adblock Plus in SeaMonkey
2. SeaMonkey restarts, profile manager pops up first
3. Adblock Plus is broken now because it runs without XPCNativeWrappers

After a second restart everything is fine again. I didn't debug it but from the code the following seems to happen when SeaMonkey is started the first time:

1. ChromeRegistry initializes, calls CheckForNewChrome()
2. CheckForNewChrome calls LoadInstallDataSource(), this sets XPCNativeWrappers setting for all packages already in chrome.rdf (Adblock Plus is not there yet)
3. CheckForNewChrome calls ProcessNewChromeBuffer which in turn calls InstallPackage and registers Adblock Plus
4. Profile manager UI opens and triggers content policies - Adblock Plus component initializes, loads some JavaScript files from chrome://adblockplus/. These files don't get flagged as requiring XPCNativeWrappers since the package hasn't been flagged yet.
5. Profile initializes, LoadProfileDataSource() is called - and sets XPCNativeWrappers setting for all packages again, this time including Adblock Plus.

So any files loaded after step 5 have XPCNativeWrappers enabled for them (I checked this) while the ones that loaded before it don't. This should be easily fixed, InstallPackage (or rather InstallProvider) needs to call nsIXPConnect.flagSystemFilenamePrefix for the new package. After all, this is a public interface and packages could even be added after the profile loads. Leaving them without protection isn't nice.

Unfortunately I don't see a work-around for this issue, nsIXPConnect.flagSystemFilenamePrefix is not accessible from JavaScript.

Comment 1

[Axel Hecht]

Neil and Benjamin may know something about this. At least it Neil did the patch in bug 281988, and Benjamin reviewed it.

XPFE chrome registry bugs are not likely to get good traction in the RDF component, is there a better place to put those?

Comment 2

[[e-mail]]

Sigh. I had a similar set of problems with bug 161751 and bug 284521 :-(

Comment 3

[[e-mail]]

Attachment: Proposed patch

* Broke out the xpcNativeWrapper test into a separate method
* Call it after programmatically installing chrome
* Call it after loading the profile
* Call it after checking for new chrome

This also fixes the case of starting Profile Manager with a new installation.

Comment 4

[Wladimir Palant]

Isn't it wasteful to call FlagXPCNativeWrappers from InstallProvider where we know exactly which packages have been added? If we install a couple of extensions at once we might end up going through the entire list of packages a dozen times. However, this case is probably rare enough so that it doesn't need optimizing...

Comment 5

[[e-mail]]

(In reply to comment #4)
>Isn't it wasteful to call FlagXPCNativeWrappers from InstallProvider where we
>know exactly which packages have been added?
Actually I don't think we do.

In fact, I'm not convinced that the provider type is accurate.

Comment 6

[Wladimir Palant]

Comment on attachment 252893 [details] [diff] [review]
Proposed patch

This fixes the problem, looks good otherwise as well. What I would prefer you to do however: add a flag to disable FlagXPCNativeWrappers calls from InstallProvider. You could then set this flag when CheckForNewChrome executes during the initialization - there will be an FlagXPCNativeWrappers call after it anyways.

Comment 7

[[e-mail]]

(In reply to comment #6)
>add a flag to disable FlagXPCNativeWrappers calls from InstallProvider.
I used the existing mBatchInstallFlushes flag, which is set by ProcessNewChromeBuffer, which is called from CheckForNewChrome.

Comment 8

[Wladimir Palant]

I see, I missed that part. Please ignore comment 4 then. However, you are assuming that a CheckForNewChrome call will always be followed by FlagXPCNativeWrappers - you should add a comment on this, just in case somebody decides to call CheckForNewChrome from a different place..

Comment 9

[Wladimir Palant]

Speaking of mBatchInstallFlushes - there are quite a few return statements between the set and unset of this flag. Under some conditions we might end up with mBatchInstallFlushes being permanently set which will both prevent this patch from working and make chrome changes disappear after a restart. I guess this needs a separate bug.

Comment 10

[jag (Peter Annema)]

Comment on attachment 252893 [details] [diff] [review]
Proposed patch

sr=jag. Feel free to move the mBatchInstallFlushes toggling out of |ProcessNewChromeBuffer()|

Comment 11

[[e-mail]]

Attachment: Patch for checkin

[jag saw this version of the patch]

I guess this is needed on branches too, right?

Comment 12

[[e-mail]]

Fix checked in to the trunk.

Comment 13

[Robert Kaiser]

Comment on attachment 253309 [details] [diff] [review]
Patch for checkin

a=me for 1.1.1 and 1.0.8

Comment 14

[[e-mail]]

Fix checked in to the branches.