The default bug view has changed. See this FAQ.

need a way to create new JS scopes and run scripts against them (same compartment)

RESOLVED FIXED in mozilla12

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: gal, Assigned: krizsa)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla12
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(1 attachment, 8 obsolete attachments)

(Reporter)

Description

6 years ago
Jetpack wants to load modules into their own scope, but doesn't need compartment boundaries in between. We should have a nice clean API for this.
A bit of additional context: this bug is the result of a conversation Andreas and I had about addressing the issue in bug 672443 that the Add-on SDK creates many compartments and thus uses a bunch of memory.

The SDK uses sandboxes to achieve module scope isolation (in particular, giving each module its own set of global objects, so modules can't modify each others' globals).  But it doesn't need the additional isolation provided by compartments (namely, an inter-compartment boundary that restricts interactions between compartments with different origins).

So it isn't necessary for SDK module scopes to be separated by compartment boundaries, and we should load them all into a single compartment to save the memory cost of compartments.  As a side effect, we might also improve the performance of inter-module interaction, since compartment boundaries apparently have some non-negligible performance cost.

So this bug is about providing an API similar to Components.utils.Sandbox that enables the SDK's module loader to create JavaScript scopes without creating a compartment for each one.
Blocks: 672443

Updated

6 years ago
Assignee: nobody → ejpbruel
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P1]

Comment 2

6 years ago
Won't this conflict with our "compartment per global" plans?
Assignee: ejpbruel → gkrizsanits
(Assignee)

Comment 3

6 years ago
I dived into the sandbox creation code a little, and it looks like a new compartment is not always created. If there is a compartment for the URI (of the passed in principal) than that will be used. The exception case is the nsSystemPrincipal where as always a new compartment is created. I think I could simply merge these compartments, the same way as they are already merged for principals with the same URI. Would that solve the problem? I'll still have to double check if there are any other principal that has no URI like the nsSystemPrincipal.

Could someone help me with some test ideas, or are there already some tests around for jetpack, for security checks? Just to be sure that this solution does not lead to any security leak I'm not aware of.
> If there is a compartment for the URI (of the passed in principal) than that
> will be used.

Shouldn't be.  In particular, the ptr passed to xpc_CreateGlobalObject is unique for each sandbox, iirc, and xpc::PtrAndPrincipalHashKey::KeyEquals uses that in addition to the principal itself.
(Assignee)

Comment 5

6 years ago
> In particular, the ptr passed to xpc_CreateGlobalObject is
> unique for each sandbox, iirc, and xpc::PtrAndPrincipalHashKey::KeyEquals
> uses that in addition to the principal itself.

Right, silly me. I looked only at the generated hash (xpc::PtrAndPrincipalHashKey::mSavedHash) and not the KeyEquals function before I posted my previous comment. So I was wrong here. But the idea is the same. 

The jetpack team is struggling that they don't need for each sandboxes a new compartment, and it has a lot of overhead. So if I create a new sandbox constructor function ('addonbox') or add a special flag to the orignal ('mergeCompartments') and if that flag is set, then instead of that uniqe ptr I just pass an nsnull to the xpc::PtrAndPrincipalHashKey. Slightly modifying the hash function and the KeyEquals function for this special case, only for sandboxes with different principals would be a new compartment created.

So this would be I think pretty much what the jetpack team want, a new global object for each sandbox is created, but no extra compartments (with all the not needed extra overhead like JIT cache, extra wrappers).

What I have to try is a., will this work? b., is it really safe for addons to get rid of those compartments...
Why pass nsnull?  Can't we just pass a pointer to something that is unique per jetpack?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
>
> So if I create a new sandbox
> constructor function ('addonbox') or add a special flag to the orignal
> ('mergeCompartments')

This reminds me of bug 673331, where the patch adds an extra arg to Cu.sandbox() to provide identifying info for the sandbox.
(Assignee)

Comment 8

6 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> Why pass nsnull?  Can't we just pass a pointer to something that is unique
> per jetpack?

Makes sense. When addons will run in their own process it will not matter, but right now it's probably a good idea to separate addons into different compartments as you suggest. Except I didn't know how to identify a sandbox call, but for that, maybe bug 673331 mentioned by Nicholas will provide a solution for me.
(Assignee)

Comment 9

6 years ago
The problem with passing something that is unique per jetpack, is that I identifying which addon/jetpack a sandbox call does belong to is not easy. Even if we can identify by passing in a name string (see bug 673331) and then look up an object from some map, anyone could simply do the same from outside the addon by creating a sandbox and using the same name.
On the other hand I don't see any important benefit for such a separation. If two compartment have the same principal the wrappers work quite transparently anyway. Blake, could you help me here if it's important to have such a separation? I tried to read up on XPCSafeJSObjectWrapper, the only wrapper what you would get in this case, but I haven't find any useful info about it, nor do I find it's implementation.
If it helps, all the sandboxes used in a single addon will be created by the same loader (a specific piece of code inside the XPI, shipped with the SDK, written by us, not by addon authors). That loader can arrange to pass the same object into each sandbox-creating call, or it could ask for some "domain"-like object once when the addon is activated, then use that object to obtain new sandboxes for each module.

I think it might actually be ok to use a unique string for this, because (I'm assuming) the sandbox-creation code is only available through XPCOM, and thus restricted to modules that declare require("chrome"). The loader has access to the unique (at least unique within a single browser) name of the addon, and could use that as a key.
(Assignee)

Comment 11

6 years ago
I would prefer to pass in an identity object instead of the string. So I don't have to maintain a map just for this on the c++ side.

Updated

6 years ago
Blocks: 636145
(Assignee)

Comment 12

6 years ago
Created attachment 554129 [details] [diff] [review]
fix with using a sandboxName as addon indentifier

In the end Warner's comment (#10) convinced me, and since there is already a solution for identifying addons (bug 673331) I ended up using the same solution. So the sandboxes with the same name and same principal will share compartments.
Attachment #554129 - Flags: review?(mrbkap)
Attachment #554129 - Flags: review?(gal)
(Assignee)

Comment 13

6 years ago
In xpc_CreateSandboxObject line 3184 the 'Identity *idPtr;' should be 
'Identity *idPtr = nsnull;'
Also there is a related bug I filled: bug 681648
(Assignee)

Comment 14

6 years ago
Created attachment 556866 [details] [diff] [review]
some fixes / cleaning up to the previous patch

I had a talk on irc with bz, and I made some changes he proposed: using nsString instead of nsCString, and storing the name on the identity object, which makes the cleaning up part a lot nicer/simplier.

Since the sandboxName turned out not to identify the addon but it identifies the module file name the sandbox was created for, I use addonId instead of sandboxId. Each addons have a unique name Id, so it should be easy and natural to use this version from jetpack.
Attachment #554129 - Attachment is obsolete: true
Attachment #554129 - Flags: review?(mrbkap)
Attachment #554129 - Flags: review?(gal)
Attachment #556866 - Flags: review?(mrbkap)
Attachment #556866 - Flags: review?(gal)
(Reporter)

Comment 15

6 years ago
Comment on attachment 556866 [details] [diff] [review]
some fixes / cleaning up to the previous patch

AddonId seems weird to me. This might be very useful in Chrome in general. Blake, what do you think?
Comment on attachment 556866 [details] [diff] [review]
some fixes / cleaning up to the previous patch

Having a dependent string outlive the thing it depends on (as here) is bad.  You probably want to declare that dependent string in that innermost block, then assign it to a stack nsString that lives long enough to be passed to the sandbox.
(Assignee)

Comment 17

6 years ago
Created attachment 557862 [details] [diff] [review]
fix for previous patch

(In reply to Boris Zbarsky (:bz) from comment #16)
> Having a dependent string outlive the thing it depends on (as here) is bad. 
> You probably want to declare that dependent string in that innermost block,
> then assign it to a stack nsString that lives long enough to be passed to
> the sandbox.

Like this? I thought that the dependent string in this case does not really depends on the jsval option, but on the string that is actually owned by the argv[1] object. But one extra memcopy here is not an issue, so I fixed it anyway just to be on the safe side.

Andreas: I'm open for any other name. Since this was an issue for the jetpack team, and there exactly the addonId will be passed here as the value, I chose that name. But if there is any other case where this feature can be used, I don't mind a name change at all.
Attachment #556866 - Attachment is obsolete: true
Attachment #556866 - Flags: review?(mrbkap)
Attachment #556866 - Flags: review?(gal)
Attachment #557862 - Flags: review?(mrbkap)
Attachment #557862 - Flags: review?(gal)
> I thought that the dependent string in this case does not really depends on the jsval
> option, but on the string that is actually owned by the argv[1] object.

It depends on the jsval option....  The new code seems fine except for the trailing space after the sandboxName declaration.
(Reporter)

Comment 19

6 years ago
Comment on attachment 557862 [details] [diff] [review]
fix for previous patch

AddonId doesn't make sense to me, we want to use this mechanism from general chrome code too. Blake, you also mentioned that you wanted an entirely different mechanism altogether. Lets touch base via vidyo today. We need clear guidance for Gabor.
(Assignee)

Comment 20

6 years ago
(In reply to Andreas Gal :gal from comment #19)

> AddonId doesn't make sense to me
It is a group of sandboxes, where compartment is shared among the ones with the same principal. So we have sandboxName, we can call this one sandboxGroup for example.

> Blake, you also mentioned that you wanted an entirely
> different mechanism altogether. 
Any info about this one? I would be really interested in it.
(Assignee)

Comment 21

6 years ago
Any update for this? Last time I talked with Blake, I was told that it is going against the long term direction we are heading, namely the compartment per global. Since jetpack should go mobile quite soon, I argued that we could still use it as a temporary solution, until the compartments will be optimized to the point where it won't matter. For jetpack modules actually we really don't need that compartment boundaries between some modules, since it comes with a cost of the extra wrappers. Plus, for a lots and lots of simple modules the 64k lower limit for compartments is a big waste right now.
Comment on attachment 557862 [details] [diff] [review]
fix for previous patch

This looks good. My only thought is that it might be easier to have the caller pass in another sandbox (or object) from which we can get the compartment/identity object. However, if JetPack's architecture makes that hard, then I'm fine going with the runtime map.
Attachment #557862 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 23

6 years ago
(In reply to Blake Kaplan (:mrbkap) from comment #22)
> Comment on attachment 557862 [details] [diff] [review] [diff] [details] [review]
> fix for previous patch
> 
> This looks good. My only thought is that it might be easier to have the
> caller pass in another sandbox (or object) from which we can get the
> compartment/identity object. However, if JetPack's architecture makes that
> hard, then I'm fine going with the runtime map.

Ok, I will check this with the jetpack team and send a final patch soon.
(Assignee)

Comment 24

6 years ago
Created attachment 567716 [details] [diff] [review]
using another sandbox instance to fetch the compartment from

It is possible. Here is a version that is using another instance of sandbox to get the identity object from (to find the compartment with it).

What we don't have this way is the name of the addon the compartment/sandbox belongs to and could be used on the about:memory page. But that is another problem, and maybe we can use the sandbox name for that so I don't mind it for now.

Implementation: What I don't like about this version is that I had to include jsobj.h for the '->compartment()' call. And that I had to get the identity object from the compartment of the sandbox, and then use that identity object later (xpc_CreateGlobalObject) to find the compartment again... I would prefer to turn the JSCompartment** arg in xpc_CreateGlobalObject into an 'in/out' argument from an 'out' argument. So it would check if it points to an nsnull or to a real compartment, and if it points to something that is not nsnull it would use it as a compartment. I just don't know if I break some practice with using a ** as an 'in/out' argument instead of a pure 'out' argument. (the danger ofc, if someone is using it with an uninitialized pointer it would crash)

Anyway let me know if you like this version or I should change something on it.
Attachment #557862 - Attachment is obsolete: true
Attachment #557862 - Flags: review?(gal)
Attachment #567716 - Flags: review?(mrbkap)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #24)
> What we don't have this way is the name of the addon the compartment/sandbox
> belongs to and could be used on the about:memory page. But that is another
> problem, and maybe we can use the sandbox name for that so I don't mind it
> for now.

I'd rather use the sandbox name for that, seeing as how that was one of the main motivations.

> Implementation: What I don't like about this version is that I had to
> include jsobj.h for the '->compartment()' call. And that I had to get the

For a little less of that "icky" feeling, you can use jsgc.h js::GetObjectCompartment here.

> identity object from the compartment of the sandbox, and then use that
> identity object later (xpc_CreateGlobalObject) to find the compartment
> again... I would prefer to turn the JSCompartment** arg in
> xpc_CreateGlobalObject into an 'in/out' argument from an 'out' argument. So
> it would check if it points to an nsnull or to a real compartment, and if it
> points to something that is not nsnull it would use it as a compartment. I
> just don't know if I break some practice with using a ** as an 'in/out'
> argument instead of a pure 'out' argument. (the danger ofc, if someone is
> using it with an uninitialized pointer it would crash)

I'd be OK with this, actually. We have other places that use in/out parameters. Just make sure that there's a big comment over the function so that potential users know to be wary.

> Anyway let me know if you like this version or I should change something on
> it.

I have a couple of nitpicks to point out, but it looks ok overall.
Comment on attachment 567716 [details] [diff] [review]
using another sandbox instance to fetch the compartment from

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

I'll r+ the next version with the in/out parameter, since I agree that'd be cleaner.

::: js/src/xpconnect/src/xpccomponents.cpp
@@ +3161,5 @@
>  NS_IMPL_ISUPPORTS0(Identity)
>  
>  nsresult
>  xpc_CreateSandboxObject(JSContext * cx, jsval * vp, nsISupports *prinOrSop, JSObject *proto,
> +                        bool wantXrays, const nsACString &sandboxName, nsISupports* identityPtr)

I know the surrounding code isn't consistent here, but this is the only parameter that uses |type* name| as opposed to |type *name|. I prefer the latter style.

@@ +3420,5 @@
>  
>              sandboxName.Adopt(tmp, strlen(tmp));
>          }
> +
> +        if (!JS_HasProperty(cx, optionsObject, "compartmentFrom", &found))

I'm not thrilled with the name, but I don't have any better suggestions. If anybody else on the CC list has one, I'd love to hear them!

@@ +3425,5 @@
> +            return NS_ERROR_INVALID_ARG;
> +
> +        if (found) {
> +            if (!JS_GetProperty(cx, optionsObject, "compartmentFrom", &option) ||
> +                !JSVAL_IS_OBJECT(option)) {

The second branch of the or statement needs to be JSVAL_IS_PRIMITIVE(option) to protect against null (which is an object, much to everybody's initial surprise).

@@ +3429,5 @@
> +                !JSVAL_IS_OBJECT(option)) {
> +                    return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);
> +            }
> +
> +            void* privateValue = JS_GetCompartmentPrivate(

I'd probably re-wrap this as:

    void *privateValue =
        JS_GetCompartmentPrivate(cx, js::GetObjectCompartment(JSVAL_TO_Object(option)));
Attachment #567716 - Flags: review?(mrbkap)
(Assignee)

Comment 27

6 years ago
Created attachment 568048 [details] [diff] [review]
polished version

(In reply to Blake Kaplan (:mrbkap) from comment #25)
> I'd be OK with this, actually. We have other places that use in/out
> parameters. Just make sure that there's a big comment over the function so
> that potential users know to be wary.

Unfortunately it did not work out. The thing is that with that identity object we might get back the same compartment but not always. If the principal object is different the compartment is not shared. An in/out compartment there would either ignore the principal and do the merge anyway, which sounds bad, or would make the API harder to use (right now it's enough to pass in the same sandbox to all the sandboxes, and sandboxes with the same principals will share compartment automatically). So I use the original approach, getting the identity object and using that, to look up the compartment. The compartmentFrom is really a bad name, and even confusing. I changed it to sameGroupAs, which is still not the best one (anyone with a better idea is more than welcome), but at least it reflects more the concept behind it: For the group there will be only one compartment per principal created.
Attachment #567716 - Attachment is obsolete: true
Attachment #568048 - Flags: review?(mrbkap)
Comment on attachment 568048 [details] [diff] [review]
polished version

This looks good to me.
Attachment #568048 - Flags: review?(mrbkap) → review+
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Depends on: 697775
Patch doesn't apply, even after fixing the filenames.
Keywords: checkin-needed
(Assignee)

Comment 30

6 years ago
Created attachment 570545 [details] [diff] [review]
Final version, in a pullable state.

I'm not sure what is the process in this case. There is no real change in this version and the previous one, it's just an updated version (in proper pullable format) Shall I ask for another review, or shall I do another pull-request?
Attachment #568048 - Attachment is obsolete: true
Blocks: 697775
No longer depends on: 697775
myk		mrbkap: do you need to re-review the updated patch in bug 677294?			2:47:34 PM
mrbkap		myk: no. I can stamp it if it would make people feel better.			2:48:34 PM
mrbkap		myk: but I trust gabor's judgement for whether it merits another review.			2:48:48 PM
Keywords: checkin-needed
(Assignee)

Comment 32

6 years ago
I discovered what was the problem with the earlier version probably. Something is wrong with the vm I'm working on and sometimes the virtual hd is getting corrupted in a random way. I've just discovered an issue like that in one of my other patches, so I will have to get rid of wm, and when I have my dev. env. up and running again without any wm involved, will have to update all my patches. This patch should be safe to be pulled since it's identical with the previous one (other than the file name and line number changes) but might contain some weird binary error (like invisible illegal character), so I don't suggest it to anyone to waste much time on it if it does not apply, in fact I think it's better to wait for a new version... Sorry for the inconveniences.
The patch applied cleanly, so I pushed it to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e672c96a4fe4
... and it contained \r's. Please avoid that.
https://hg.mozilla.org/mozilla-central/rev/e672c96a4fe4
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Keywords: checkin-needed

Comment 36

6 years ago
This needs a doc update and a test.

The patch seems to modify the Sandbox() <https://developer.mozilla.org/en/Components.utils.Sandbox> constructor to allow optional sameGroupAs property on the 2nd parameter). I'm not 100% sure as to what its value is supposed to be from the code...
Flags: in-testsuite?
Keywords: dev-doc-needed
Version: unspecified → Trunk
(Assignee)

Comment 37

6 years ago
(In reply to Nickolay_Ponomarev from comment #36)
> This needs a doc update and a test.

This is an optional optimization feature. The parameter should be another sandbox, and the result is, that the new sandbox will be created in the same sandbox group. Sandboxes from the same group share compartment per principals (only one compartment for sandboxes with system principals in the same group)

This patch might be a temporary optimization, which might be changed (removed) in the future, when compartments will be optimal enough for creating one per global. Which means I'm not sure if this should be a documented feature or something only addons will use for now. 

For the tests my plan was, to integrate this into the jetpack project, and use all the jetpack test for testing it, but I will add some stand alone tests as well this week.

Comment 38

6 years ago
> The parameter should be another sandbox
Thanks for the clarification!

> might be changed (removed) in the future
That's true for many parts of the platform :)

Note that my "this needs" comment was more of a reminder to go with the flags change and a preface for my question. I'm not in the position to tell you what to do.
(Assignee)

Comment 39

5 years ago
So as it turned out there is a problem with this patch. Which I did not detect earlier since my test was a bit unique (I had an external sandbox in my test in which the other sandboxes were created). The problem is that when someone passing in a sandbox like:

Cu.Sandbox(somePrincipal, {sameGroupAs: otherSandbox});

then this code:

+            void* privateValue =
+                JS_GetCompartmentPrivate(cx,GetObjectCompartment(JSVAL_TO_OBJECT(option)));

will ignore the context of otherSandbox and will work with the context where the Cu.Sandbox is called. 'option' here will be an ObjectProxyClass. So my question is (blake, bz, or anyone who could help me) how could I get the context of the otherSandbox here instead of the context of the proxy object?

Also, I just realized that this can be NULL:
+            identity = compartmentPrivate->key->GetPtr();
in the case when the sameGroupAs property is an object from a compartment that does not belong to a sandbox, in which case I think we should just throw.

Finally, I really want to write a simple test for this, to avoid anything like this in the future, does anyone know how can I detect (in js) if two sandbox with the same principal share compartment or not?
Oops, I really should have seen that. You can call js::UnwrapObject on the incoming object. It's also probably worth insisting that either it or its global object's JS class is sandbox_class.
(Assignee)

Comment 41

5 years ago
Created attachment 575856 [details] [diff] [review]
fix for the previous version

This patch fixes the problem, but I still don't know any good test for this patch(except debugging manually), Blake: I could not avoid this time including the jsobj.h since the context we have here and the unwrapped object would fail the assertSameCompartment tests. The include of GlobalObject.h could have been avoided by casting the GlobalObject* to JSObject*, but I was suggested not to do a c type cast, so I just included that too.
Attachment #570545 - Attachment is obsolete: true
Attachment #575856 - Flags: review?(mrbkap)
(Assignee)

Comment 42

5 years ago
So I just checked this patch today, while wrote a test for it, and realized that it does not compile any longer: linker error because of the functions GlobalObject.h functions. So... would you mind not doing the sandbox_clas check? It would mean that any object from the other sandbox's compartment can be the input argument.
We should be able to put anything we need into jsfriendapi, no?
Comment on attachment 575856 [details] [diff] [review]
fix for the previous version

You shouldn't include vm/GlobalObject.h; you won't be able to after bug 692277.
Attachment #575856 - Flags: feedback-
Followup from IRC: JSAPI has different APIs that allow different levels of intimacy. We're trying to move XPConnect off of using spidermonkey internals directly, and instead using public APIs. jsfriendapi is a catch-all API we use when XPConnect needs something that spidermonkey doesn't otherwise want to expose.

Ms2ger is probably the best person to help you out with which functions you should be using. And you guys are in similar timezones too! ;-)
(Assignee)

Comment 46

5 years ago
Created attachment 583147 [details] [diff] [review]
fixed linking issues

Thanks, I fixed the linking problem I had with using jsfriendapi.
Attachment #575856 - Attachment is obsolete: true
Attachment #575856 - Flags: review?(mrbkap)
Attachment #583147 - Flags: review?(mrbkap)
(Assignee)

Updated

5 years ago
Attachment #583147 - Attachment is patch: true
(Reopening to keep on radar, as essential followup patch awaits review and landing.)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 583147 [details] [diff] [review]
fixed linking issues

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

Sorry for the delay. With the bug being closed, I didn't see this in my review queue.

::: js/xpconnect/src/XPCComponents.cpp
@@ +3241,2 @@
>              void* privateValue =
> +                JS_GetCompartmentPrivate(cx,GetObjectCompartment(unwrapped));

Mind adding a space after the , here?
Attachment #583147 - Flags: review?(mrbkap) → review+
Comment on attachment 583147 [details] [diff] [review]
fixed linking issues

>+                GetObjectJSClass(global) != &SandboxClass) {
>+                    return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);

Indented too far.

>-            if (!compartmentPrivate || !compartmentPrivate->key)
>+            if (!compartmentPrivate || !compartmentPrivate->key) {
>                 return ThrowAndFail(NS_ERROR_INVALID_ARG, cx, _retval);
>+            }

XPConnect uses JS style, which doesn't brace single-line ifs. (3 times)
(Assignee)

Comment 50

5 years ago
Created attachment 585298 [details] [diff] [review]
fixed patch

(In reply to Blake Kaplan (:mrbkap) from comment #48)
> 
> Sorry for the delay. With the bug being closed, I didn't see this in my
> review queue.

Sorry about that and thanks Myk.

(In reply to Ms2ger from comment #49)
> Indented too far.
> XPConnect uses JS style, which doesn't brace single-line ifs. (3 times)

The file and even that function itself is quite inconsistent about that but I'll keep that in mind for the future.

Could someone help me landing this patch? I don't have access to tinderbox nor to central...
Attachment #583147 - Attachment is obsolete: true
Attachment #585298 - Flags: review+
I'll land it.
https://hg.mozilla.org/mozilla-central/rev/941801e7439c
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla10 → mozilla12
You need to log in before you can comment on or make changes to this bug.