The default bug view has changed. See this FAQ.

nsIChromeRegistrySea.installPackage should respect xpcNativeWrappers flag

RESOLVED FIXED

Status

SeaMonkey
Security
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Wladimir Palant, Assigned: neil@parkwaycc.co.uk)

Tracking

({fixed-seamonkey1.0.8, fixed-seamonkey1.1.1})

Trunk
fixed-seamonkey1.0.8, fixed-seamonkey1.1.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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

10 years ago
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?
(Assignee)

Comment 2

10 years ago
Sigh. I had a similar set of problems with bug 161751 and bug 284521 :-(
(Assignee)

Comment 3

10 years ago
Created attachment 252893 [details] [diff] [review]
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #252893 - Flags: superreview?(jag)
Attachment #252893 - Flags: review?(trev)
(Reporter)

Comment 4

10 years ago
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...
(Assignee)

Comment 5

10 years ago
(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.
(Reporter)

Comment 6

10 years ago
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.
Attachment #252893 - Flags: review?(trev) → review+
(Assignee)

Comment 7

10 years ago
(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.
(Reporter)

Comment 8

10 years ago
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..
(Reporter)

Comment 9

10 years ago
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

10 years ago
Comment on attachment 252893 [details] [diff] [review]
Proposed patch

sr=jag. Feel free to move the mBatchInstallFlushes toggling out of |ProcessNewChromeBuffer()|
Attachment #252893 - Flags: superreview?(jag) → superreview+
(Assignee)

Updated

10 years ago
Assignee: neil → dveditz
Status: ASSIGNED → NEW
Component: RDF → Security
Product: Core → Mozilla Application Suite
QA Contact: rdf → seamonkey
(Assignee)

Comment 11

10 years ago
Created attachment 253309 [details] [diff] [review]
Patch for checkin

[jag saw this version of the patch]

I guess this is needed on branches too, right?
Assignee: dveditz → neil
Attachment #252893 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #253309 - Flags: approval-seamonkey1.1.1?
Attachment #253309 - Flags: approval-seamonkey1.0.8?
(Assignee)

Comment 12

10 years ago
Fix checked in to the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 13

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

a=me for 1.1.1 and 1.0.8
Attachment #253309 - Flags: approval-seamonkey1.1.1?
Attachment #253309 - Flags: approval-seamonkey1.1.1+
Attachment #253309 - Flags: approval-seamonkey1.0.8?
Attachment #253309 - Flags: approval-seamonkey1.0.8+
(Assignee)

Comment 14

10 years ago
Fix checked in to the branches.
Keywords: fixed-seamonkey1.0.8, fixed-seamonkey1.1.1
You need to log in before you can comment on or make changes to this bug.