352791 - Permission denied to get property XULElement.ownerDocument

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P5)

Description

[Ginn Chen]

When nsAccessibilityService::GetShellFromNode calls
  aNode->GetOwnerDocument(getter_AddRefs(domDoc));
and aNode is nsXPCWrapped.

I got
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Permission denied to get property XULElement.ownerDocument'
when calling method: [nsIDOMXULSelectControlItemElement::ownerDocument]" 
nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>" 
data: no]
************************************************************

If I add nsCxPusher here, the message will be gone.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the basic problem is that when we're calling out into an nsXPCWrappedJS we use whatever cx happens to be on the stack, falling back on the safe JSContext if there isn't one.  But in this case the nsXPCWrappedJS is an XBL binding for a chrome XULElement, so when we try to get the subject principal we end up with the hidden window, which is not allowed to access chrome.

It would make more sense to me to ask the nsXPCWrappedJS |wrapper| in nsXPCWrappedJSClass::CallMethod whether it has an associated JSContext.  Sorta like the |this| translator thing we have.  For example, content could do what nsCxPusher::Push does (QI to contents and documents and whatnot, looking for a window).

Comment 2

[Johnny Stenback (:jst)]

Is this something that breaks common functionality? If so, please re-nominate, otherwise this doesn't sound like a blocker bug.

Comment 3

[Mike Shaver (:shaver emeritus)]

This is one of several bugs (such as inexplicable NS_ERROR_FAILURE when working with XHR, or the "permission denied to get <property>" for some dropdowns or the location bar, etc.) that make our platform hard to work with reliably, and difficult to diagnose actually application problems on.  I need to find those bugs and mark them consistently, but for now this one can draw my ire! :)

I don't know if fixing this is a blocking bug, but if not then having a clear workaround for it that we can document and point people at should be, IMO.  Renominating, but with no anger in my heart!

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Blake, you totally asked for stuff to work on the other day. Here you go :)

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

How do I reproduce this, to test potential fixes?

Comment 6

[Ginn Chen]

If you have Ubuntu 7.04, you can try
Enable assistive technologies in Gnome Prefs.
Run orca behind.
Run Firefox, go to Preferences->Main tab, focus the combo box "When Minefield starts"
You should see the message in JS console.

Comment 7

[Damon Sicore (:damons)]

Targeting to A6 per conversation with Blake.

Comment 8

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

This sucks a little bit to fix. Really, connecting JSContexts to principals is badly incorrect, and we should fix that (with a principals stack, which Boris has suggested before). Since we can't do that for 1.9, we'll have to find another way to fix this.

The approach I'm going to try here is to try to associate each XPCNativeScope with an XPCContext, the same way that the DOM does. Then, I'll be able to use whatever context is associated with the root JSObject's scope in nsXPCWrappedJSClass::CallMethod and friends.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Blake, you saw comment 1, right?  I think what you propose would work, and I'm not sure which is harder, but if you need something to fall back on...

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Boris, the problem that I'm trying to solve is the "associate |wrapper| with a JSContext." Currently, the only way to get a JSContext out of a window is to QI to nsIScriptGlobalObject (which isn't threadsafe, but that's another discussion) and get the context from there. Unfortunately, nsIScriptGlobalObject depends on widget (for nsEventStatus), so I can't include nsIScriptGlobalObject.h, and it isn't possible to forward declare enums.

Because of this, I have to find another way to get a JSContext from a global object, and the easiest (and threadsafe!) way that I could think of was to duplicate what the DOM currently does. I'm not thrilled with it, but it should work.

Comment 12

[Boris Zbarsky [:bzbarsky]]

I guess I was thinking in terms of a new interface a la nsIXPCFunctionThisTranslator, with the implementation living in DOM code.  But that still wouldn't be threadsafe, as you point out.  And would only work for certain DOM objects (which means you should only be touching them on the main thread, but anyway)....

So I guess the more general solution you're doing is not that much more complicated if at all, and should work better.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Here's a first stab at a patch. With it, I'm still seeing spurious property access denied errors, but I can't reliably reproduce them, and the testcases in this bug are fixed. I was worried about context/scope destruction order, so I add all scopes that are associated with an XPCContext to a linked list in the context, and notify the linked list of the context's destruction at the proper time.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 270777 [details] [diff] [review]
patch v1

I don't know if Boris is doing reviews right now...

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 270777 [details] [diff] [review]
patch v1


Seems pretty fool-proof, but I'd love to know the story behind the as-yet unknown paths to wrongful access denial you're still seeing even with this patch.

>@@ -848,6 +853,8 @@ private:
>     LangType mCallingLangType;
>     PRUint16 mSecurityManagerFlags;
>     JSPackedBool mMarked;
>+    // A linked list of scopes to notify when we are destroyed.
>+    PRCList mScopes;

Nit: blank line before the comment (see elsewhere in xpcprivate.h for similar).

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 17

[Michael Ventnor]

This might have caused a big leak regression.

RLk:240KB
Lk:4.12MB

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #17)
> This might have caused a big leak regression.

I have been looking into the regressions caused by my checkins today, backing them out in the order that I feel is likely to fix the regressions. In particular, this patch does not add any new roots, change JavaScript, refcount any objects, or, in general, do anything that should affect leak numbers in any way. Of course, if my other backouts don't fix the regressions, this is next on the list to back out.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 270777 [details] [diff] [review]
patch v1

>+GetContextFromObject(JSObject *obj)
>+{
>+    // In order to get a context, we need a context.

Cryptic!  Very nice!  But what does it mean?  ;)

Other than that, looks ok, though I haven't double-checked to make sure of all the details...

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

This was next on the list, backed out.

Comment 21

[Ginn Chen]

I applied this patch to my build, it crashed at exit.
@NS_LogCOMPtrRelease_P

Because of the leak issue?

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

It's possible. I didn't run into anything like that. Do you know what object was being destroyed when it crashed?

Comment 23

[Ginn Chen]

Attachment: crash stack

stack attached.

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

That stack looks like bug 382350.

Comment 25

[Ben Turner (not reading bugmail, use the needinfo flag!)]

jst asked me to look into the leaks reported here. I did five trace-malloc-enabled bloatcycle runs of the unpatched and patched versions, here are the leaked bytes:

Unpatched: 1464227, 1577891, 1569563, 1566198, 1576054
  Patched: 1453767, 1581684, 1580653, 1546668, 1573024

Avg unpatched: 1550786.6
  Avg patched: 1547159.2 (-0.23%)

It certainly doesn't seem like this patch will increase leaks.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

mrbkap, are you ok with me relanding this?

Comment 27

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Updated patch

This patch is updated to current trunk, carrying all flags forward.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

bent, if you have the cycles to try to land this again, I'd really appreciate it.

Comment 29

[Blake Kaplan (:mrbkap) (inactive)]

Also, I never attached a patch that dealt with brendan's nit in comment 15, which should be picked before final checkin.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Relanded. Early results look good, so resolving. I'll keep an eye on the leak numbers today.

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, and I fixed the nit from comment 15.

Comment 32

[Steve England [:stevee]]

I think this may have caused bug 412598.

Comment 33

[Reed Loden [:reed]]

and maybe bug 413093

Comment 34

[Lo]

bug 418426 also appears to be a side-effect of this patch. Would appreciate help in resolving that bug too.