Closed Bug 735280 Opened 13 years ago Closed 12 years ago

Remove the Components object's dependency on caps.

Categories

(Core :: XPConnect, defect)

Product:
Core
Component:
XPConnect
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: mrbkap, Assigned: gkrizsanits)

References

(Depends on 1 open bug)

Details

Attachments

(3 files, 17 obsolete files)

part 2: adding pre-create to nsXPComponents
6.97 KB, patch
bholley
: review+
Details | Diff | Splinter Review
part 3: Components object specific wrapper
16.85 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
part 1: connect XPCWrappedNativeScope and Components
7.06 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review 
The Components object is currently exposed to every scope possible with the intention that we only expose certain properties to content (see nsXPCComponents::CanGetProperty). The way that this works is that we implement nsISecurityCheckedComponent, we don't do any security wrapping (in the COW sense) and we wait until XPCWrappedNative::CallMethod to realize at the last minute that the 'this' object is an nsISecurityCheckedComponent and that we might want to throw a security error (all this in the getter for .classes and friends).

We should find a way of implementing this behavior that doesn't rely on the security check in XPCWrappedNative::CallMethod.

See also: bug 693733 and bug 429070
Blocks: 735281
Assignee: nobody → gkrizsanits
So we agreed to move this logic from caps to a custom wrapper. The wrapper should be like a SOW in a sense that it is in the same compartment as the wrapped object. Now I've got pretty far with a patch and now struggling with the problem that WrapperFacory is automatically unwrapping things, before wrapping it to another compartment. So I'm not sure how to / where handle the case of exposing the component to another compartment. If I prevent the WrapperFactory to unwrap this particular wrapper, would wrapper chaining work? (so for example a COW that proxys to this special wrapper that proxys to the real Component object) Or shall I do something else?
In general, I think we should avoid having wrappers around wrappers. Instead, I think we should just have a version of this wrapper that works cross-compartment (like the location object wrapper, actually).

That having been said, I'm not sure if it's necessary... It seems like the code in WrapperFactory::PrepareForWrapping might simply always create a new JSObject in each compartment that the Components object is exposed to, and those objects should be wrapped (same-compartment, of course).
We should never need to expose the Components object cross-compartment. Every global has its own Components object, so there should never be a reason to do that.

We could throw, but I like Blake's solution better:

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> That having been said, I'm not sure if it's necessary... It seems like the
> code in WrapperFactory::PrepareForWrapping might simply always create a new
> JSObject in each compartment that the Components object is exposed to, and
> those objects should be wrapped (same-compartment, of course).

So if you try to access a Components object cross-compartment, you just end up with your own Components object.
(In reply to Bobby Holley (:bholley) from comment #3)
> We should never need to expose the Components object cross-compartment.
> Every global has its own Components object, so there should never be a
> reason to do that.

I seem to recall jetpack doing exactly this, but the details escape me for the moment (there was an include or something to get access to the Components object).

The thing is that you don't actually get *your* Components object, you get *another* Components object. I don't see too much of a problem with this, though, since the behavior of the components object is entirely dynamic based on the components manager.
(In reply to Blake Kaplan (:mrbkap) from comment #4)
> (In reply to Bobby Holley (:bholley) from comment #3)
> > We should never need to expose the Components object cross-compartment.
> > Every global has its own Components object, so there should never be a
> > reason to do that.
> 
> I seem to recall jetpack doing exactly this, but the details escape me for
> the moment (there was an include or something to get access to the
> Components object).

Oh hm - maybe because we don't init the Components goop on sandboxes? Maybe we could have an option for that..

> The thing is that you don't actually get *your* Components object, you get
> *another* Components object. I don't see too much of a problem with this,
> though, since the behavior of the components object is entirely dynamic
> based on the components manager.

I guess so. I think it'd still be nice to avoid maintaining a cross-compartment and same-compartment version of the wrapper, since conceptually it seems like everyone who needs a Components object can just have their own. Maybe the jetpack/sandbox case sinks that though?
(In reply to Bobby Holley (:bholley) from comment #5)
> Oh hm - maybe because we don't init the Components goop on sandboxes? Maybe
> we could have an option for that..

We actually do. I think there's a bug filed to add an option to avoid adding it :)

> I guess so. I think it'd still be nice to avoid maintaining a
> cross-compartment and same-compartment version of the wrapper, since
> conceptually it seems like everyone who needs a Components object can just
> have their own. Maybe the jetpack/sandbox case sinks that though?

I don't really see why it's that much of a pain. I guess if we do this too much, there will be a codesize hit, but other than that it's a little bit of well-understood template goopery, no?
(In reply to Blake Kaplan (:mrbkap) from comment #6)
> I don't really see why it's that much of a pain.

It's not. If the easiest thing is just to do things LW/XLW style, I'm fine with that too.
(In reply to Blake Kaplan (:mrbkap) from comment #2)
> That having been said, I'm not sure if it's necessary... It seems like the
> code in WrapperFactory::PrepareForWrapping might simply always create a new
> JSObject in each compartment that the Components object is exposed to, and
> those objects should be wrapped (same-compartment, of course).

I more or less understand the LW/XLW style, but I don't grasp this approach...

I have some other questions too, I checked how does LW work under the hood and found these guys:
 http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedNative.cpp#1672  http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.cpp#1084

and I'm not sure why do we need them exactly...

The other thing is a case like when the user set some flags in about:config and then enables Cu to be used from scratchpad for example... For that I assume I should somehow call nsScriptSecurityManager::IsCapabilityEnabled with some arguments... right?
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> I have some other questions too, I checked how does LW work under the hood
> and found these guys:
>  http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/
> XPCWrappedNative.cpp#1672 

This is the main thoroughfare where objects get wrapped. Here, you need to check if we're wrapping for content and if so, stick one of your wrappers around the outgoing object.

> http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCConvert.
> cpp#1084

This function is called in specific cases for DOM nodes (when a DOM node changes documents/windows we need to move its corresponding object to the new scope). Because this can't happen for Components objects, you don't have to worry about it.

> The other thing is a case like when the user set some flags in about:config
> and then enables Cu to be used from scratchpad for example... For that I
> assume I should somehow call nsScriptSecurityManager::IsCapabilityEnabled
> with some arguments... right?

Yes, you'll base that decision on AccessCheck.cpp:PermitIfUniversalXPConnect.

One thing to note here is that we don't actually need any wrappers around Components in chrome.
(In reply to Blake Kaplan (:mrbkap) from comment #9)

> This is the main thoroughfare where objects get wrapped. Here, you need to
> check if we're wrapping for content and if so, stick one of your wrappers
> around the outgoing object.

Specifically, the issue here is that we're dealing with same-compartment security wrappers, not cross-compartment ones. So we can't rely on the cross-compartment wrapping callbacks that take us into WrapperFactory::Rewrap.
(In reply to Blake Kaplan (:mrbkap) from comment #9)
> Yes, you'll base that decision on AccessCheck.cpp:PermitIfUniversalXPConnect.

This is what I thought as well... but unfortunately when I set the flag devtools.chrome.enabled to true in about:config, in scratchpad Cu works fine in the old version, but PermitIfUniversalXPConnect fails... Any clue?

> 
> One thing to note here is that we don't actually need any wrappers around
> Components in chrome.

Right now I add the wrapper in every case, because I thought setting properties are not allowed on it even in chrome, but I now realized that I was wrong, so I can get rid of it I guess.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11)
> This is what I thought as well... but unfortunately when I set the flag
> devtools.chrome.enabled to true in about:config, in scratchpad Cu works fine
> in the old version, but PermitIfUniversalXPConnect fails... Any clue?

Oops, yes. The pref you're setting changes the principal of the scratchpad, You need to find a mochitest that uses ...enablePrivilege("UniversalXPConnect");

> Right now I add the wrapper in every case, because I thought setting
> properties are not allowed on it even in chrome, but I now realized that I
> was wrong, so I can get rid of it I guess.

Yeah, don't worry about chrome at all, this is entirely about content not being able to do things.
Attached patch first try... (obsolete) — — Splinter Review 
First of all this patch does not do much yet since I have not turned off the security check in XPCWrappedNative::CallMethod. And I'm really not sure about the ReparentWrapperIfFound part... But I think it's easier to discuss it further by looking at what I have.
Attachment #608308 - Flags: review?(mrbkap)
Comment on attachment 608308 [details] [diff] [review]
first try...

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

This is a good start. I have a bunch of questions below, though. I'd also like to see a mochitest dedicated to testing some of the edge cases here.

::: js/xpconnect/src/XPCComponents.cpp
@@ +4251,5 @@
> +    bool isChrome = xpc::AccessCheck::isChrome(js::GetObjectCompartment(aGlobal));
> +    
> +    JSObject* safeObj = isChrome ? obj : xpc::WrapperFactory::WrapComponentsObject(ccx, obj);
> +
> +    return safeObj && JS_DefinePropertyById(ccx, aGlobal, id, OBJECT_TO_JSVAL(safeObj),

Rather than put the wrapping code here, it seems like it'd be a little nicer to put it into XPCConvert::NativeInterface2JSObject, so we catch odd edge-cases as well.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1671,5 @@
>                      if (xpc::WrapperFactory::IsLocationObject(flat)) {
>                          newwrapper = xpc::WrapperFactory::WrapLocationObject(ccx, newobj);
>                          if (!newwrapper)
>                              return NS_ERROR_FAILURE;
> +                    } else if (xpc::WrapperFactory::IsComponentsObject(flat)) {

You should be able to assert that this never happens.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +606,5 @@
> +                             Permission &perm) 
> +{  
> +  perm = DenyAccess;
> +  JSAutoEnterCompartment ac;
> +  if (!ac.enter(cx, wrapper))

Why is this necessary?

@@ +615,5 @@
> +      if (JS_FlatStringEqualsAscii(flatId, "isSuccessCode") ||
> +          JS_FlatStringEqualsAscii(flatId, "lookupMethod") ||
> +          JS_FlatStringEqualsAscii(flatId, "interfaces") ||
> +          JS_FlatStringEqualsAscii(flatId, "interfacesByID") ||
> +          JS_FlatStringEqualsAscii(flatId, "results"))

I wonder if it's worth using interned IDs here. Probably not, I suppose.

@@ +616,5 @@
> +          JS_FlatStringEqualsAscii(flatId, "lookupMethod") ||
> +          JS_FlatStringEqualsAscii(flatId, "interfaces") ||
> +          JS_FlatStringEqualsAscii(flatId, "interfacesByID") ||
> +          JS_FlatStringEqualsAscii(flatId, "results"))
> +              perm = PermitPropertyAccess;      

This seems misindented.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +317,5 @@
>                      wrapper = &NoWaiverWrapper::singleton;
>                  }
>              }
>          }
> +    } else if (IsComponentsObject(obj)) {

Are you sure this can happen? I still think that the code in WrapperFactory::PrepareForWrapping prevents this. Need a testcase!

@@ +477,5 @@
> +JSObject *
> +WrapperFactory::WrapComponentsObject(JSContext *cx, JSObject *obj)
> +{
> +    JSObject *wrapperObj =
> +        Wrapper::New(cx, obj, JS_GetPrototype(obj), JS_GetGlobalForObject(cx, obj), //<--clean this and check if obj is the Component

What does this comment mean?

@@ +479,5 @@
> +{
> +    JSObject *wrapperObj =
> +        Wrapper::New(cx, obj, JS_GetPrototype(obj), JS_GetGlobalForObject(cx, obj), //<--clean this and check if obj is the Component
> +                     &FilteringWrapper<SameCompartmentSecurityWrapper,
> +                     ComponentsObjectPolicy>::singleton);

Uber-nit: I'd prefer to see the ComponentsObjectPolicy line line up with SameCompartmentSecurityWrapper.
Attachment #608308 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #14)
> like to see a mochitest dedicated to testing some of the edge cases here.

Sure, I planned to write one, just I forgot to ask this question. What would be a good test case for the code in ReparentWrapperIfFound? It's not clear for me in what cases is that function called.

> > +{  
> > +  perm = DenyAccess;
> > +  JSAutoEnterCompartment ac;
> > +  if (!ac.enter(cx, wrapper))
> 
> Why is this necessary?

Right, so this is one more thing I wanted to ask you about. I've seen it to be used in most of the 'check' functions and I didn't want to forget about double checkint this with you if it's needed or not. Can you explain me why do we need it in SameOriginOrCrossOriginAccessiblePropertiesOnly::check? 

> I wonder if it's worth using interned IDs here. Probably not, I suppose.

I don't suppose that this code is very performance critical, so I'd say if it makes the code looks nicer sure, otherwise probably it does not worth it.

> ::: js/xpconnect/wrappers/WrapperFactory.cpp
> @@ +317,5 @@
> >                      wrapper = &NoWaiverWrapper::singleton;
> >                  }
> >              }
> >          }
> > +    } else if (IsComponentsObject(obj)) {
> 
> Are you sure this can happen? I still think that the code in
> WrapperFactory::PrepareForWrapping prevents this. Need a testcase!

hmmm... no I'm not. But I tried a test where I pass a Components object from chrome to a content sandbox and it worked. But I did not check if this code was triggered. I will.

> > +        Wrapper::New(cx, obj, JS_GetPrototype(obj), JS_GetGlobalForObject(cx, obj), //<--clean this and check if obj is the Component
> 
> What does this comment mean?

Ahh, yeah... sorry about that. Nevermind, I just forgot to remove that comment.

Ok, so I'll fix the rest and file a patch hopefully tomorrow with a test.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #15)
> Right, so this is one more thing I wanted to ask you about. I've seen it to
> be used in most of the 'check' functions and I didn't want to forget about
> double checkint this with you if it's needed or not. Can you explain me why
> do we need it in SameOriginOrCrossOriginAccessiblePropertiesOnly::check? 

Ah, right, it's so we throw the exception in the caller's scope so they can inspect the error. Mind adding a comment in both places? So it's OK to leave it in. However, why don't you return true in the if statement and then the rest of the function would simply be "return PermitIfUniversalXPConnect(...)"? Either way, we try to avoid else after return in Mozilla code.
(In reply to Blake Kaplan (:mrbkap) from comment #16)

> it in. However, why don't you return true in the if statement and then the
> rest of the function would simply be "return
> PermitIfUniversalXPConnect(...)"? Either way, we try to avoid else after
> return in Mozilla code.

whoops... yeah, it made sense at some point in a previous version of the code, but then I ended up with this version and my eyes just skipped this code when I reviewed  and tried cleaning up things. But thanks for the notice!
Attached patch 2nd try (obsolete) — — Splinter Review 
So this is still not a final version, nor a final test. Just made some progress and have some questions. As it turns out I had to understand a lot about wrapper creation, and finally I did what Blake suggested in Comment 2. So there is no cross compartment version of the wrapper this time. I'm not sure if we really need any wrapper code anywhere else than the PrepareForWrapping and the AttachNewComponentsObject. So I'd like to get rid of the wrapping in XPCWrappedNative.cpp and in XPCConvert.cpp. Is there any use case why we should keep those? 

The last test is failing and I'm not sure why. It's probably not related to my wrapper, since there should be no Components specific wrapper created. The equality test fails because the wrapped native is the same but the wrappers are not the same objects (that should not happen in same compartment right?)
Attachment #608308 - Attachment is obsolete: true
Attachment #609330 - Flags: review?(mrbkap)
Comment on attachment 609330 [details] [diff] [review]
2nd try

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

Looks like we're getting there...

::: js/xpconnect/src/XPCConvert.cpp
@@ +1104,5 @@
>              }
>  
>              flat = sowWrapper;
> +        } else if (xpc::WrapperFactory::IsComponentsObject(flat) &&
> +          !xpc::AccessCheck::isChrome(js::GetObjectCompartment(flat))) { 

Nit: the ! in the second line here should line up with the 'x' in the first line.

@@ +1106,5 @@
>              flat = sowWrapper;
> +        } else if (xpc::WrapperFactory::IsComponentsObject(flat) &&
> +          !xpc::AccessCheck::isChrome(js::GetObjectCompartment(flat))) { 
> +            JSObject* componentWrapper = xpc::WrapperFactory::WrapComponentsObject(cx, flat);
> +            NS_ASSERTION(componentWrapper, "component wrapper creation failed");

Before landing, this should turn into a runtime check for failure instead of an assertion.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1671,5 @@
>                      if (xpc::WrapperFactory::IsLocationObject(flat)) {
>                          newwrapper = xpc::WrapperFactory::WrapLocationObject(ccx, newobj);
>                          if (!newwrapper)
>                              return NS_ERROR_FAILURE;
> +                    } else if (xpc::WrapperFactory::IsComponentsObject(flat)) {

I still want to know if this actually happens. I think you should be able to assert not...

@@ +2317,5 @@
>                                        ccx.GetWrapper()->GetClassInfo(),
>                                        ccx.GetMember()->GetName(),
>                                        ccx.GetWrapper()->GetSecurityInfoAddr()))) {
>          // the security manager vetoed. It should have set an exception.
> +        //return false;

Why?

::: js/xpconnect/tests/unit/test_components.js
@@ +24,5 @@
> +do_check_eq(rv, undefined);
> +
> +sb3.C = Components;
> +rv = Cu.evalInSandbox("C.utils", sb3);
> +do_check_eq(rv, Cu);

Yeah, this test should pass. More debugging needed! :-)

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +621,5 @@
> +  }
> +  if (perm == PermitPropertyAccess)
> +      return true;
> +
> +  return PermitIfUniversalXPConnect(cx, id, act, perm);  // Deny

This almost does the right thing, however, I think we should throw if we don't have access. This will return undefined instead. PermitIfUniversalXPConnect was written for ChromeObjectWrappers, so you'll need to add an argument there or change act to confuse it into throwing.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +240,5 @@
>          if (newwn->GetSet()->GetInterfaceCount() < wn->GetSet()->GetInterfaceCount())
>              newwn->SetSet(wn->GetSet());
>      }
>  
> +    if (IsComponentsObject(obj) && !AccessCheck::isChrome(js::GetObjectCompartment(scope))){

This is needed because we pass false for aAllowWrapping above, right? If so, it needs a comment.

This also will create multiple Components wrapper wrappers if the wrapped native we found had already been created. Once bug 739796 lands, that'll be easier to fix.
Attachment #609330 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #19)
> Comment on attachment 609330 [details] [diff] [review]
> > +  return PermitIfUniversalXPConnect(cx, id, act, perm);  // Deny
> 
> This almost does the right thing, however, I think we should throw if we
> don't have access. This will return undefined instead.
> PermitIfUniversalXPConnect was written for ChromeObjectWrappers, so you'll
> need to add an argument there or change act to confuse it into throwing.

This one caused me some headache... So my problem is that if I throw for a Get property because of the nature of the filtering wrapper it will also throw for a Has property, since there is no difference between the two at this level (ComponentsObjectPolicy::check). But in the old version 
[if ("classes" in Components)] did NOT throw, and because of this check: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#647 pretty much all test are failing now... 

So I could invent an Action::Has but not sure if it worth the trouble. Or I could just invent an extra flag for the check function for distinguish between Get and Has, then all the rest of the check function could stay the same, and I would use that flag only in this wrapper. Would work but it's ugly... Or the version I would prefer is to throw anyway, and change the SimpleTest.js check to a try/catch block. I don't know what else would this brake.

> 
> This also will create multiple Components wrapper wrappers if the wrapped
> native we found had already been created. Once bug 739796 lands, that'll be
> easier to fix.

Will this solve it? Or shall I wait for bug 739796 to land?

    // Since the previous WrapNativeToJSVal call prevents security wrappers to be created
    // we have to deal with the ComponentsObjectWrapper here
    if (IsComponentsObject(obj) && !AccessCheck::isChrome(js::GetObjectCompartment(scope))){
        JSAutoEnterCompartment ac;
        if (!ac.enter(cx, scope))
            return nsnull;
        
        // update the wn
        wn = static_cast<XPCWrappedNative *>(xpc_GetJSPrivate(obj));

        JSObject* componentsWrapper = wn->GetWrapper();
        if (!componentsWrapper) {
            componentsWrapper = WrapComponentsObject(cx, obj);
            if (!componentsWrapper)
              return nsnull;

            wn->SetWrapper(componentsWrapper);
        }
        return componentsWrapper;
    }
Attached patch 3rd try (obsolete) — — Splinter Review 
Ok, this might be it. So no throwing for property get attempts. Fixed the failing tests, I'm not sure about the reftest, because for some reason I could not test it locally (just pushed to try, fingers crossed...).
Attachment #609330 - Attachment is obsolete: true
Attachment #611786 - Flags: review?(mrbkap)
Comment on attachment 611786 [details] [diff] [review]
3rd try

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

So close! I want to see test_bug246699 test something useful and r+ after that, I promise.

::: caps/tests/mochitest/test_bug246699.html
@@ +1,4 @@
>  <!DOCTYPE HTML>
>  <html>
>  <!--
>  https://bugzilla.mozilla.org/show_bug.cgi?id=246699

With this patch, this test should officially be impossible to write: we're trying to make sure that when caps throws an exception that it throws a true exception object instead of a string. I think it might be better to have us throw an a real security exception (as opposed to a type error) and do the test on that object instead.

::: js/xpconnect/src/XPCConvert.cpp
@@ +1106,5 @@
>  
>              flat = sowWrapper;
> +        } else if (xpc::WrapperFactory::IsComponentsObject(flat) &&
> +                   !xpc::AccessCheck::isChrome(js::GetObjectCompartment(flat))) { 
> +            JSObject* componentWrapper = xpc::WrapperFactory::WrapComponentsObject(cx, flat);

You need to check that wrapper->GetWrapper() returns null here.
Attachment #611786 - Flags: review?(mrbkap)
Attached patch might be final (obsolete) — — Splinter Review 
(In reply to Blake Kaplan (:mrbkap) from comment #22)
> Comment on attachment 611786 [details] [diff] [review]
> 3rd try
> 
> Review of attachment 611786 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So close! I want to see test_bug246699 test something useful and r+ after
> that, I promise.

We'll see :) I'm still not so sure about this test. This version seemed to be the simplest, but you might not like it... I don't know how could get a XOW here
which would throw on a get request too. Since that would be the same exception I guess this version should do it.

Anyway I pushed it to try just in case: https://tbpl.mozilla.org/?tree=Try&rev=f0df84a107ae
Attachment #611786 - Attachment is obsolete: true
Attachment #612241 - Flags: review?(mrbkap)
Comment on attachment 612241 [details] [diff] [review]
might be final

Perfect. It might be worth adding a comment that setting Components.classes triggers an exception while getting it doesn't, but I leave that up to you.

Thanks a ton for your work here!
Attachment #612241 - Flags: review?(mrbkap) → review+
Comment on attachment 612241 [details] [diff] [review]
might be final

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

You wanted nits? :)

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1667,5 @@
>  
>                  JSObject *ww = wrapper->GetWrapper();
>                  if (ww) {
>                      JSObject *newwrapper;
> +                    NS_ASSERTION(!xpc::WrapperFactory::IsComponentsObject(flat), 

MOZ_ASSERT?

@@ +1673,4 @@
>                      if (xpc::WrapperFactory::IsLocationObject(flat)) {
>                          newwrapper = xpc::WrapperFactory::WrapLocationObject(ccx, newobj);
>                          if (!newwrapper)
> +                            return NS_ERROR_FAILURE;                  

Fix the trailing whitespace, please.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +621,5 @@
>  }
>  
> +bool
> +ComponentsObjectPolicy::check(JSContext *cx, JSObject *wrapper, jsid id, Wrapper::Action act,
> +                             Permission &perm) 

Looks like it needs one more space?

@@ +635,5 @@
> +            JS_FlatStringEqualsAscii(flatId, "lookupMethod") ||
> +            JS_FlatStringEqualsAscii(flatId, "interfaces") ||
> +            JS_FlatStringEqualsAscii(flatId, "interfacesByID") ||
> +            JS_FlatStringEqualsAscii(flatId, "results"))
> +            perm = PermitPropertyAccess;      

More trailing spaces. Also:

            JS_FlatStringEqualsAscii(flatId, "results"))
        {
            perm = PermitPropertyAccess;
        }

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +247,5 @@
>          newwn->SetSet(unionSet);
>      }
>  
> +    // Since the previous WrapNativeToJSVal call prevents security wrappers to be created
> +    // we have to deal with the ComponentsObjectWrapper here

Period at the end of the sentence.

@@ +256,5 @@
> +        
> +        // update the wn
> +        wn = static_cast<XPCWrappedNative *>(xpc_GetJSPrivate(obj));
> +
> +        JSObject* componentsWrapper = wn->GetWrapper();

* on the right
Attached patch ready to go (obsolete) — — Splinter Review 
Cleaned up version.
Attachment #612241 - Attachment is obsolete: true
Attachment #612842 - Flags: review+
Keywords: checkin-needed
This unfortunately is going to need a rebase on top of my same-compartment security wrapper stuff from bug 739796 that just landed. :-(

Can you do that, gabor?
Keywords: checkin-needed
Attached patch rebased (obsolete) — — Splinter Review 
Sure. Care to take a look at it? Also pushed it to try just in case, but it should just work.

https://tbpl.mozilla.org/?tree=Try&rev=90fa2671bae2
Attachment #612842 - Attachment is obsolete: true
Attachment #613597 - Flags: review?(bobbyholley+bmo)
Attached patch rebased+fixed (obsolete) — — Splinter Review 
I forgot to update a file in the previous patch, here is a version that actually compiles. 

https://tbpl.mozilla.org/?tree=Try&rev=67186131acdd
Attachment #613597 - Attachment is obsolete: true
Attachment #613620 - Flags: review?(bobbyholley+bmo)
Attachment #613597 - Flags: review?(bobbyholley+bmo)
Comment on attachment 613620 [details] [diff] [review]
rebased+fixed

>diff --git a/js/xpconnect/src/XPCComponents.cpp b/js/xpconnect/src/XPCComponents.cpp

>-    return NS_SUCCEEDED(wrapper->GetJSObject(&obj)) &&
>-           obj && JS_DefinePropertyById(ccx, aGlobal, id, OBJECT_TO_JSVAL(obj),
>-                                        nsnull, nsnull,
>-                                        JSPROP_PERMANENT | JSPROP_READONLY);
>+    NS_ENSURE_SUCCESS(wrapper->GetJSObject(&obj), false);
>+    
>+    bool isChrome = xpc::AccessCheck::isChrome(js::GetObjectCompartment(aGlobal));
>+    JSObject* safeObj = isChrome ? obj : xpc::WrapperFactory::WrapComponentsObject(ccx, obj);

This should probably go through GetSameCompartmentSecurityWrapper. If it doesn't, we'll miss the call to SetWrapper and could end up with two different same-compartment Components wrappers.

>@@ -2234,16 +2236,21 @@ XPCWrappedNative::GetSameCompartmentSecu
>     if (xpc::WrapperFactory::IsLocationObject(flat)) {
>         wrapper = xpc::WrapperFactory::WrapLocationObject(cx, flat);
>         if (!wrapper)
>             return NULL;
>     } else if (NeedsSOW()) {
>         wrapper = xpc::WrapperFactory::WrapSOWObject(cx, flat);
>         if (!wrapper)
>             return NULL;
>+    } else if (xpc::WrapperFactory::IsComponentsObject(flat) &&
>+               !xpc::AccessCheck::isChrome(js::GetObjectCompartment(flat))) {
>+        wrapper = xpc::WrapperFactory::WrapComponentsObject(cx, flat);
>+        if (!wrapper)
>+            return NULL;

Why is the chrome check necessary here? We handle that earlier in the function.


>+    if (JSID_IS_STRING(id) && act == Wrapper::GET) {
>+        JSFlatString *flatId = JSID_TO_FLAT_STRING(id);
>+        if (JS_FlatStringEqualsAscii(flatId, "isSuccessCode") ||
>+            JS_FlatStringEqualsAscii(flatId, "lookupMethod") ||
>+            JS_FlatStringEqualsAscii(flatId, "interfaces") ||
>+            JS_FlatStringEqualsAscii(flatId, "interfacesByID") ||
>+            JS_FlatStringEqualsAscii(flatId, "results"))
>+        {
>+            perm = PermitPropertyAccess;
>+        }
>+    }
>+    if (perm == PermitPropertyAccess)
>+        return true;

Why not get rid of the if() and move the |return true| under the |perm = PermitPropertyAccess|?

>+    // we should throw for invalid getter attempts too, so we pass in Wrapper::SET
>+    return PermitIfUniversalXPConnect(cx, id, act, perm);  // Deny

I don't understand this comment.

>diff --git a/js/xpconnect/wrappers/WrapperFactory.cpp b/js/xpconnect/wrappers/WrapperFactory.cpp

>+    // Since the previous WrapNativeToJSVal call prevents security wrappers to be created
>+    // we have to deal with the ComponentsObjectWrapper here.

I don't understand this comment either. I don't think we always call WrapNativeToJSVal immediately before calling PrepareForWrapping, so maybe this comment needs to go somewhere else?

>+    if (IsComponentsObject(obj) && !AccessCheck::isChrome(js::GetObjectCompartment(scope))){
>+        JSAutoEnterCompartment ac;
>+        if (!ac.enter(cx, scope))
>+            return nsnull;
>+        
>+        // update the wn
>+        wn = static_cast<XPCWrappedNative *>(xpc_GetJSPrivate(obj));
>+
>+        JSObject *componentsWrapper = wn->GetWrapper();
>+        if (!componentsWrapper) {
>+            componentsWrapper = WrapComponentsObject(cx, obj);
>+            if (!componentsWrapper)
>+                return nsnull;
>+
>+            wn->SetWrapper(componentsWrapper);
>+        }

This duplicates GetSameCompartmentSecurityWrapper.

>+        return componentsWrapper;

So is our cross-compartment story that we return a wrapper to the SCSW in the host compartment (similar to what we do for same-origin LWs, now)? If so, this seems kind of suboptimal to me, but maybe I don't know the whole backstory. What's our cross-compartment situation? It seems like it's exercised, at least, in the xpcshell test above.


>diff --git a/js/xpconnect/wrappers/WrapperFactory.h b/js/xpconnect/wrappers/WrapperFactory.h

>     // Wrap a (same compartment) object in a SOW.
>     static JSObject *WrapSOWObject(JSContext *cx, JSObject *obj);
>+
>+    // Return true if this is a location object.
>+    static bool IsComponentsObject(JSObject *obj);
>+
>+    // Wrap Component object (same compartment)

Nit: Components plural. While we're at it, just make this "Wrap a (same compartment) Components object." to match the SOW comment.
Attachment #613620 - Flags: review?(bobbyholley+bmo) → review-
Attached patch update (obsolete) — — Splinter Review 
(In reply to Bobby Holley (:bholley) from comment #30)
> Comment on attachment 613620 [details] [diff] [review]
> rebased+fixed
> 
> This should probably go through GetSameCompartmentSecurityWrapper. If it
> doesn't, we'll miss the call to SetWrapper and could end up with two
> different same-compartment Components wrappers.

Whoops, true.

> Why is the chrome check necessary here? We handle that earlier in the
> function.

Sorry, I missed that.

> Why not get rid of the if() and move the |return true| under the |perm =
> PermitPropertyAccess|?
> 
> >+    // we should throw for invalid getter attempts too, so we pass in 
> I don't understand this comment.

Both these things are left overs and make no sense any longer, thanks for finding them.

> >+    // Since the previous WrapNativeToJSVal call prevents security wrappers to be created
> >+    // we have to deal with the ComponentsObjectWrapper here.
> 
> I don't understand this comment either. I don't think we always call
> WrapNativeToJSVal immediately before calling PrepareForWrapping, so maybe
> this comment needs to go somewhere else?
> 
> >+    if (IsComponentsObject(obj) && !AccessCheck::isChrome(js::GetObjectCompartment(scope))){
> >+        JSAutoEnterCompartment ac;
> >+        if (!ac.enter(cx, scope))
> >+            return nsnull;
> >+        
> >+        // update the wn
> >+        wn = static_cast<XPCWrappedNative *>(xpc_GetJSPrivate(obj));
> >+
> >+        JSObject *componentsWrapper = wn->GetWrapper();
> >+        if (!componentsWrapper) {
> >+            componentsWrapper = WrapComponentsObject(cx, obj);
> >+            if (!componentsWrapper)
> >+                return nsnull;
> >+
> >+            wn->SetWrapper(componentsWrapper);
> >+        }
> 
> This duplicates GetSameCompartmentSecurityWrapper.

So the comment is about the WrapNativeToJSVal call a few lines above in the PrepareForWrapping function itself. It is not entirely clear to me why we call it with prevent security wrappers flag here, but I'm afraid if I simply call GetSameCompartmentSecurityWrapper here, it will break many things (not in my case, but for Location object for instance). So this is the reason I duplicated code. If you have a nicer solution I'm all for it, I couldn't find any yet...

> So is our cross-compartment story that we return a wrapper to the SCSW in
> the host compartment (similar to what we do for same-origin LWs, now)? If
> so, this seems kind of suboptimal to me, but maybe I don't know the whole
> backstory. What's our cross-compartment situation? It seems like it's
> exercised, at least, in the xpcshell test above.

We really have no cross-compartment version. So the PrepareForWrapping code always creates a jsobject for each compartment the Components object is exposed to. If it's a chrome compartment, we don't need ComponentsWrapper for that object, if it's not a chrome compartment it does need one.

I attached the current version of the patch let me know if you think further improvements are required.
Attachment #613620 - Attachment is obsolete: true
Attachment #614338 - Flags: review?(bobbyholley+bmo)
Comment on attachment 614338 [details] [diff] [review]
update

So I think it makes the most sense for nsXPCComponents to implement PreCreate, so that we only ever have one per scope. Here's what you need to do, probably as a separate patch underneath this one:

1 - Modify nsXPCComponents to have a weak XPCWrappedNativeScope* pointer that it takes in the constructor.

2 - Modify XPCWrappedNativeScope to have:
nsRefPtr<nsXPCComponents> mComponents.

3 - Rename nsXPCComponents::AttachNewComponentsObject to nsXPCComponents::AttachComponentsObject. Have it use the existing mComponents (rather than new-ing) if it exists, otherwise it creates it and caches it in mComponents.

4 - In the XPCWrappedNativeScope destructor, null out mComponents->mScope. Make a comment that this is probably unnecessary, since the Components object should die along with the scope, but just in case. You might need to make XPCWNScope a friend class of nsXPCComponents here.

5 - Add WANT_PRECREATE to the nsXPCComponents flags, and implement it to return the global object from the scope, or throw (and print a warning that this shouldn't happen) if the scope is null.


Sound good? Once this is done, we can count on having only one nsXPCComponents WN per scope. This means that you don't need to worry about same-compartment security wrappers in PrepareForUnwrapping. Just add an XCW or something for the cross-compartment case.
Attachment #614338 - Flags: review?(bobbyholley+bmo)
Attached patch 2nd part (obsolete) — — Splinter Review 
So I created this patch on top of the previous as a followup.
Attachment #615292 - Flags: review?(bobbyholley+bmo)
Can you separate the MAP_ENTRY_COMPOSITE stuff into a separate patch underneath this one? I'm not an XPCOM peer.
Depends on: 737559
Comment on attachment 615292 [details] [diff] [review]
2nd part

On IRC, I suggested that this would be best split into 3 patches:

1 - The NS_INTERFACE_MAP_ENTRY_COMPOSITE change (with r? bsmedberg)
2 - Making nsXPCScritable implement precreate
3 - The rest of the patch
Attachment #615292 - Flags: review?(bobbyholley+bmo)
So it turned out that this patch requires the Components object to implement nsXPCClassInfo. Instead of copy pasting the custom QI code from nsDOMClassInfo, I introduced a new macro for it that matches the rest of macros in this area.
Attachment #614338 - Attachment is obsolete: true
Attachment #615292 - Attachment is obsolete: true
Attachment #615654 - Flags: review?(benjamin)
bholley: I'm sure you meant this with patch 2 in your comment.
Attachment #615656 - Flags: review?(bobbyholley+bmo)

        Attachment #615658 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  
Comment on attachment 615656 [details] [diff] [review]
part 2: adding pre-create to nsXPComponents

This patch seems kind of screwy - there are bits from part 1, and it seems to be based on top of something else that had a SetComponents on XPCWNScope.

These patches should all stack on top each each other, with each qpush compiling and passing tests. I'd suggest using a dedicated repo per bug/project to avoid doing a bunch of costly rebuilds. ;-)

        Attachment #615656 -
        Flags: review?(bobbyholley+bmo)

        Attachment #615658 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  


  
(In reply to Bobby Holley (:bholley) from comment #39)
> This patch seems kind of screwy - there are bits from part 1, and it seems
Crap. I attached the wrong patch, not sure how I could have missed it... 

> These patches should all stack on top each each other, with each qpush
> compiling and passing tests. I'd suggest using a dedicated repo per
> bug/project to avoid doing a bunch of costly rebuilds. ;-)

Sure I'm already doing that. Since on windows even a tiny rebuild takes quite some time I simply use dedicated repos for each bugs for now. When I have all the patched r+ I will push them to try.

        Attachment #615656 -
        Attachment is obsolete: true

        Attachment #616054 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  
Comment on attachment 616054 [details] [diff] [review]
part 2: adding pre-create to nsXPComponents

>+NS_IMETHODIMP
>+nsXPCComponents::PreCreate(nsISupports *nativeObj, JSContext *cx, JSObject *globalObj, JSObject **parentObj)
>+{
>+  // this should never happen
>+  if (!mScope)
>+      return NS_ERROR_FAILURE;

Let's emit an NS_WARNING if this happens.

>+protected:
>+  virtual void PreserveWrapper(nsISupports *aNative)
>+  {
>+  }
>+  
>+  virtual PRUint32 GetInterfacesBitmap()
>+  {
>+      return 0; 
>+  }
>+

Just make these one-liners to save space, since they don't do anything:

virtual void PreserveWrapper(nsISupports *aNative) { }
virtual PRUint32 GetInterfacesBitmap() { return 0; }

Also, add a comment explaining that we need to implement nsXPCClassInfo (rather than pure nsIXPCScriptable) in order to get PreCreate, and we need these for ClassInfo, even though they don't really apply to this situation.

Looks great! r=bholley

        Attachment #616054 -
        Flags: review?(bobbyholley+bmo) → review+

    
    

    
  
Comment on attachment 616054 [details] [diff] [review]
part 2: adding pre-create to nsXPComponents

Or wait, no. This is still wrong, because as of this patch there's no such thing as mScope.

        Attachment #616054 -
        Flags: review+ → review-

    
    

    
  
Comment on attachment 615654 [details] [diff] [review]
part 1: adding QI macros for nsXPCClassInfo special case

This macro looks to me like it is identical to NS_IMPL_QUERY_BODY_AMBIGUOUS, and serves exactly the same function. So I don't think we need the new macro unless there is something I'm missing.

        Attachment #615654 -
        Flags: review?(benjamin) → review-

    
    

    
  
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #43)
> Comment on attachment 615654 [details] [diff] [review]
> part 1: adding QI macros for nsXPCClassInfo special case
> 
> This macro looks to me like it is identical to NS_IMPL_QUERY_BODY_AMBIGUOUS,
> and serves exactly the same function. So I don't think we need the new macro
> unless there is something I'm missing.

the NS_GET_IID part is different... they look very similar but not the same. If I used the NS_IMPL_QUERY_BODY_AMBIGOUS macro here with args (nsXPCClassInfo, nsIClassInfo) it would first check for the NS_GET_IID(nsXPCClassInfo) which is correct but then it would cast it first to nsXPCClassInfo then to nsXPCClassInfo and the later one implements nsISupports twice so that would be ambiguous. If I use it with args (nsIClassInfo, nsXPCClassInfo) then the casting part would work fine, but the NS_GET_IID(nsIClassInfo) part would not be valid. This new macro only make sense for special interfaces that implements multiple sub interfaces so I names it composite (because the ambiguous was already taken).

    
    

    
  


  

        Attachment #615654 -
        Attachment is obsolete: true

        Attachment #617196 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  


  
As we agreed on irc using GetHelperForLanguage for this part.

        Attachment #616054 -
        Attachment is obsolete: true

        Attachment #617197 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  


  

        Attachment #615658 -
        Attachment is obsolete: true

        Attachment #617198 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  
Comment on attachment 617196 [details] [diff] [review]
part 1: connect XPCWrappedNativeScope and Components


> 
>+    // This should not be necessary, since the Components object should die
>+    // with the scope but just in case.
>+    if (mComponents)
>+        mComponents->mScope = nsnull;
>+
>     // XXX we should assert that we are dead or that xpconnect has shutdown
>     // XXX might not want to do this at xpconnect shutdown time???
>-    NS_IF_RELEASE(mComponents);
>+    mComponents = nsnull;

Would you mind killing these XXX comments? It would be nice of the null-out of mComponents came immediately after the mScope null-out (visually).

> 
> private:
>+    friend class XPCWrappedNativeScope;

Looking at this again, I think a NullOutScope() method would be better here than unfettered friend access. Sorry for the bad advice the first time around. :-(

Looks great! r=bholley

        Attachment #617196 -
        Flags: review?(bobbyholley+bmo) → review+

    
    

    
  
Comment on attachment 617197 [details] [diff] [review]
part 2: adding pre-create to nsXPComponents

nice.

        Attachment #617197 -
        Flags: review?(bobbyholley+bmo) → review+

    
    

    
  
Comment on attachment 617198 [details] [diff] [review]
part 3: Components object specific wrapper


>                 JSObject *ww = wrapper->GetWrapper();
>                 if (ww) {
>                     JSObject *newwrapper;
>+                    MOZ_ASSERT(!xpc::WrapperFactory::IsComponentsObject(flat), 
>+                                 "Components object should never get here");

Nit: The string should be aligned here.

>diff --git a/js/xpconnect/wrappers/WrapperFactory.cpp b/js/xpconnect/wrappers/WrapperFactory.cpp
>--- a/js/xpconnect/wrappers/WrapperFactory.cpp
>+++ b/js/xpconnect/wrappers/WrapperFactory.cpp
>@@ -338,16 +338,19 @@ WrapperFactory::Rewrap(JSContext *cx, JS
>                     typedef XrayWrapper<CrossCompartmentWrapper> Xray;
>                     usingXray = true;
>                     wrapper = &Xray::singleton;
>                 } else {
>                     wrapper = &NoWaiverWrapper::singleton;
>                 }
>             }
>         }
>+    } else if (IsComponentsObject(obj)) {
>+        wrapper = &FilteringWrapper<CrossCompartmentSecurityWrapper, 
>+                                    ComponentsObjectPolicy>::singleton;

Hm. So, I understand why you put this here: if we're not chrome, we want an XCW when accessing a Components object cross-origin. But putting it here makes the logic of this function really confusing.

Currently, we ask:
1 - Is the caller chrome? If so, grant it unfettered access, modulo some Xray stuff.
2 - If not, is the object chrome? If so, watch out, since content is touching chrome. Default to ExposedPropertiesOnly.
3 - Otherwise, are we same-origin? If so, allow some kinds of transparent access.
4 - Otherwise, we're cross-origin content<->content. Apply the cross-origin filtering wrapper.

So putting XCW logic between 1 and 2 doesn't fit in conceptually, even though it's correct.

An important observation here is that content should never be able to get its hands on a chrome Components object. And even if it does, the default wrapper will be ExposedPropertiesOnly, which will deny access to everything. So I don't think we have to worry about case 2.

The same applies to case 4, except that the access will be blocked by CrossOriginAccessiblePropertiesOnly instead of ExposedPropertiesOnly.

So we really only need XCW in (3). Can we put it there?


>     static JSObject *WrapSOWObject(JSContext *cx, JSObject *obj);
>+
>+    // Return true if this is a location object.

Typo: 'Components object'.

r- because I want to double-check the eventual XCW placement. But in general, this patch looks great. Huzzah! \o/

        Attachment #617198 -
        Flags: review?(bobbyholley+bmo) → review-

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #50)

> 
> An important observation here is that content should never be able to get
> its hands on a chrome Components object. And even if it does, the default
> wrapper will be ExposedPropertiesOnly, which will deny access to everything.
> So I don't think we have to worry about case 2.
> 


In general I really didn't like putting that "else if (IsComponentsObject(obj))" there in the first place. Your suggestion changes the patch conceptually slightly but in general I would not have problem with it, and in fact I would like to place that code in the case 3 branch only. So I tried it out and what surprised me is that for ExposedPropertiesOnly (step 2) I expereienced different behavior than expected. if I import a Components object of a chrome sandbox into a none chrome sandbox I can just simply access it's utils property (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp#524) of course XPCWrappedNative::CallMethod will still throw but still, this sounds really bad for me. Can someone explain me why we let this property access through and if it's a bug is?

    
    

    
  
Arg! Looks like this is bug 553102, which never landed. I might try to find some cycles to fix this myself. I think it's pretty important for security...

In the mean time, here's what we should do here:

1 - Do what I previously suggested, but add a duplicate check for Components in case 2 (content accessing chrome). Make a comment that it can be removed post bug 553102. I'd still prefer to do it this way (two separate checks for IsComponents) than to break up the case logic.

2 - File a followup bug for removing it blocking bug 553102, and CC me.

    
    

    
  


  
I fixed a build stupid issue I have noticed because on windows it just compiled... Anyway, this seems to be good now:
https://tbpl.mozilla.org/?tree=Try&rev=1956f1ad4ca1

        Attachment #617196 -
        Attachment is obsolete: true

        Attachment #618054 -
        Flags: review+

    
    

    
  


  
for part 3, I had to change the tests, but now it looks good: https://tbpl.mozilla.org/?tree=Try&rev=9873d725e4d6

        Attachment #617198 -
        Attachment is obsolete: true

        Attachment #618057 -
        Flags: review?(bobbyholley+bmo)

    
    

    
  
(In reply to Bobby Holley (:bholley) from comment #52)
> 2 - File a followup bug for removing it blocking bug 553102, and CC me.

Let me know if it's alright now, and then I'll create and add some info about the test that should be changed.

    
    

    
  
Comment on attachment 618057 [details] [diff] [review]
part 3: Components object specific wrapper

yay! r=bholley

        Attachment #618057 -
        Flags: review?(bobbyholley+bmo) → review+

    
    

    
  
Nit: "none chrome" should be "non-chrome". This appears in a few places in the test.

    
    

    
  


  
Fixed the non-chrome typos. And hurray! :)

        Attachment #618057 -
        Attachment is obsolete: true

        Attachment #618113 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c5557c4d1fe

../../../../js/xpconnect/src/XPCWrappedNativeScope.cpp: In member function 'void XPCWrappedNativeScope::DebugDump(PRInt16)':
../../../../js/xpconnect/src/XPCWrappedNativeScope.cpp:918:80: error: cannot pass objects of non-trivially-copyable type 'class nsRefPtr<nsXPCComponents>' through '...'
Target Milestone: mozilla15 → ---

    
    

    
  


  
Sorry about that... This should work: https://tbpl.mozilla.org/?tree=Try&rev=f27c816d5d7a

        Attachment #618054 -
        Attachment is obsolete: true

        Attachment #618326 -
        Flags: review+

    
  
Keywords: checkin-needed

    
    

    
  
And backed out again due to jsreftest failures (which appeared on the Try run as well...).
https://hg.mozilla.org/integration/mozilla-inbound/rev/40b1bc1bdba1

REFTEST INFO | Loading a blank page
REFTEST TEST-START | http://10.250.48.212:30102/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-328897.js | 738 / 1136 (64%)
REFTEST TEST-UNEXPECTED-PASS | http://10.250.48.212:30102/jsreftest/tests/jsreftest.html?test=js1_5/Regress/regress-328897.js | JS_ReportPendingException should  item 1
Flags: in-testsuite+
Target Milestone: mozilla15 → ---

    
    

    
  
Ryan, were all three parts backed out? A manual inspection seems to imply so, but the commit message only mentions backing out a0b3af4ac9f5.

The original landing and backout has merged to mozilla-central, so if parts2/3 were in fact left in, then they are now on m-c too.

    
    

    
  
All 3 parts were backed out. Sorry for not making that more clear in the commit message.

    
    

    
  
I need some help here. So this test is likely to fail because we throw now sooner than before (from the wrapper instead of a bit later from XPCWrappedNative::CallMethod) I guess some navigation occurs before the onerror would be called, despite the fact that we throw. But the fact that this happens only on android and that I don't know much about this part of the code, nor can I reproduce it locally it makes me really hard to come up with some solution. Anyone who might be able to help me out?

    
    

    
  
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #67)
> I need some help here. So this test is likely to fail because we throw now
> sooner than before (from the wrapper instead of a bit later from
> XPCWrappedNative::CallMethod) I guess some navigation occurs before the
> onerror would be called, despite the fact that we throw. But the fact that
> this happens only on android and that I don't know much about this part of
> the code, nor can I reproduce it locally it makes me really hard to come up
> with some solution. Anyone who might be able to help me out?

Looks like bz knows about the relevant test here, and is probably the best person to illuminate the issue. bz, do you have a cycle or two to help gabor out here?

    
    

    
  
Throwing from the wrapper vs throwing from Components.classes shouldn't really affect this test, I don't think.

Why is this an unexpected _pass_?

> I guess some navigation occurs before the onerror would be called

Seems pretty unlikely, given the test structure, unless things are _really_ broken.

    
    

    
  
A better question.  What exception string is being thrown with your changes?  Does it match the "expect" regexp?

    
    

    
  
(In reply to Boris Zbarsky (:bz) from comment #70)
> A better question.  What exception string is being thrown with your changes?
> Does it match the "expect" regexp?

So the test passes on all other platforms except on android, and the error message is not platform specific as far as I can tell. It is from AccessCheck::deny with SET operation. "Permission denied to access property '%hs'"

So you're saying it that if the error message is different, then the test times out instead of failing? I would expect in case of an exception with different message to fail, but I might be wrong.

    
    

    
  
Do you have a link to a log on Android where the patch caused the test to fail?

I would think that changing

expect = /(Script error.|Permission denied for <file:\/\/> to get property XPCComponents.classes)/;

to include an additional pattern which matches your failure message (assuming it is an indication of success) would be sufficient for the test to pass.

    
    

    
  
(In reply to Bob Clary [:bc:] from comment #72)
> Do you have a link to a log on Android where the patch caused the test to
> fail?

https://tbpl.mozilla.org/?tree=Try&rev=f27c816d5d7a

> 
> I would think that changing
> 
> expect = /(Script error.|Permission denied for <file:\/\/> to get property
> XPCComponents.classes)/;

This pattern looks more like the old exception that was thrown by caps (security manager), now the exception that is thrown in this case should match the pattern I'm using. But I still don't get why does it pass on all other platform, am I missing something obvious here?

    
    

    
  
Yeah, I pasted what is in the tree now. From your patch the change is:

    2.12 -  expect = /(Script error.|Permission denied for <file:\/\/> to get property XPCComponents.classes)/;
    2.13 +  expect = /(Script error.|Permission denied to access property 'classes')/;

Ah. The test has the annotation // |reftest| fails-if(browserIsRemote). So you are passing now. The original annotation in the manifest was

changeset:   83172:6cff9824c2c1
user:        Phil Ringnalda <philringnalda@gmail.com>
date:        Tue Dec 20 21:58:43 2011 -0800
summary:     Bug 624621 - mark the test as failing on remote-jsreftests, where the message talks about the IP address and port rather than 'file://', r=mbrubeck

Can you just remove the annotation that it fails on remote?

    
    

    
  
(In reply to Bob Clary [:bc:] from comment #74)
> Ah. The test has the annotation // |reftest| fails-if(browserIsRemote). So
> you are passing now. The original annotation in the manifest was

Thanks a lot Bob, I owe you one! It's more than likely that this is going to be it. It would have take me quite some time to find this flag for sure...

    
    

    
  
If you can give it a try push today verify that it works, I can take care of pushing it to inbound tonight. No need to attach another patch for just the manifest change.

    
    

    
  
(In reply to Ryan VanderMeulen from comment #76)
> If you can give it a try push today verify that it works, I can take care of
> pushing it to inbound tonight. No need to attach another patch for just the
> manifest change.

Sure. Can someone point me out where is this manifest file? It used to be jstests.list in the folder of the regression tests, but the file is gone. And I could not find any replacement manifest file that has a reference to this particular test in it. (the old list file did have this flag indeed)

    
    

    
  
bug 735549 removed the external manifests for the jsreftests and moved the annotations into each individual test file. Look at the top of js/src/tests/js1_5/Regress/regress-328897.js for the line  // |reftest| fails-if(browserIsRemote) and remove it.

    
    

    
  
bc: thanks

Ryan: it's still not finished so I just leave the link it here now, and will check it if it passed later: https://tbpl.mozilla.org/?tree=Try&rev=d3827223490f

    
    

    
  
Ryan: it looks good to me so far... Just let me know if you need another version of the patch with the manifest change. Crossing fingers it'll work this time...

    
    

    
  
Looks like the Android issue is indeed fixed, However, it appears that there are crashes on OSX with the following stack:

 0  libSystem.B.dylib + 0x19c0a
    rbx = 0x000cfe50   r12 = 0x70cd6998   r13 = 0x70cd6d68   r14 = 0xffffffff
    r15 = 0x70cd6da8   rip = 0x8748dc0a   rsp = 0x000cfce8   rbp = 0x000cfe90
    Found by: given as instruction pointer in context
 1  libSystem.B.dylib + 0x1badc
    rip = 0x8748fadd   rsp = 0x000cfcf0
    Found by: stack scanning
 2  libmozglue.dylib!arena_run_split [_string.h : 80 + 0x15]
    rip = 0x00008fa5   rsp = 0x000cfd40
    Found by: stack scanning
 3  libmozglue.dylib!arena_run_alloc [jemalloc.c : 3571 + 0x15]
    rbx = 0x0031d000   r12 = 0x00001000   r13 = 0x00000001   r14 = 0x00000001
    r15 = 0x00000000   rip = 0x0000c851   rsp = 0x000cfdc0   rbp = 0x00200040
    Found by: call frame info
 4  libmozglue.dylib!je_calloc [jemalloc.c : 1674 + 0x7]
    rbx = 0x00001000   r12 = 0x00200040   r13 = 0x0001e000   r14 = 0x00000001
    r15 = 0x00000000   rip = 0x0000ea73   rsp = 0x000cfe20   rbp = 0x00001000
    Found by: call frame info
 5  0x7fff70cd6997
    rbx = 0x0021fff6   r12 = 0x00000000   r13 = 0x0001e000   r14 = 0x00000001
    r15 = 0x00000000   rip = 0x70cd6998   rsp = 0x000cfe50   rbp = 0x00000000
    Found by: call frame info
 6  libSystem.B.dylib + 0x1b7b3
    rip = 0x8748f7b4   rsp = 0x000cfea0
    Found by: stack scanning
 7  libSystem.B.dylib + 0x1b2dd
    rip = 0x8748f2de   rsp = 0x000cfed0
    Found by: stack scanning
 8  libSystem.B.dylib + 0x1ac07
    rip = 0x8748ec08   rsp = 0x000cff30
    Found by: stack scanning
 9  libSystem.B.dylib + 0x1aaa4
    rip = 0x8748eaa5   rsp = 0x000cff60

I don't see those failures in your previous push, however, which makes me wonder if it's an issue with the changeset it was pushed on top of. I'm going to push it to Try again.

    
    

    
  
yaaay! thanks Ryan, crossing fingers...

    
    

    
  
Should this be marked fixed?
Depends on: 751627

    
    

    
  
We're investigating why locking prefs no longer works (bug 776490).

Is it possible this could be the culprit? We're now seeing these errors parsing mozilla.cfg files:

-134424768[7ffff6b65150]: general.config.filename = mozilla.cfg
-134424768[7ffff6b65150]: evaluating .cfg file mozilla.cfg with obscureValue 0
-134424768[7ffff6b65150]: ###!!! ASSERTION: About to do a meaningless security check!: '!AccessCheck::callerIsChrome()', file /home/komat/tmp965/src/js/xpconnect/wrappers/AccessCheck.cpp, line 373
-134424768[7ffff6b65150]: ###!!! ASSERTION: Script compiled without principals!: 'Error', file /home/komat/tmp965/src/caps/src/nsScriptSecurityManager.cpp, line 2121
-134424768[7ffff6b65150]: ###!!! ASSERTION: About to do a meaningless security check!: '!AccessCheck::callerIsChrome()', file /home/komat/tmp965/src/js/xpconnect/wrappers/AccessCheck.cpp, line 373
-134424768[7ffff6b65150]: ###!!! ASSERTION: Script compiled without principals!: 'Error', file /home/komat/tmp965/src/caps/src/nsScriptSecurityManager.cpp, line 2121

    
    

    
  
It's unlikely. I commented over there.

    
    

    
  
Bobby, Gabor, is there a reason this bug is still open?  See comment 85.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: