Closed
Bug 352791
Opened 18 years ago
Closed 17 years ago
Permission denied to get property XULElement.ownerDocument
Categories
(Core :: XPConnect, defect, P5)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: ginnchen+exoracle, Assigned: mrbkap)
References
Details
Attachments
(2 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bent.mozilla
:
review+
bent.mozilla
:
superreview+
|
Details | Diff | Splinter Review |
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•18 years ago
|
||
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).
Flags: blocking1.9?
Updated•18 years ago
|
Assignee: dbradley → nobody
Comment 2•18 years ago
|
||
Is this something that breaks common functionality? If so, please re-nominate, otherwise this doesn't sound like a blocker bug.
Flags: blocking1.9? → blocking1.9-
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!
Flags: blocking1.9- → blocking1.9?
Blake, you totally asked for stuff to work on the other day. Here you go :)
Assignee: nobody → mrbkap
Flags: blocking1.9? → blocking1.9+
Assignee | ||
Comment 5•18 years ago
|
||
How do I reproduce this, to test potential fixes?
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•18 years ago
|
||
Targeting to A6 per conversation with Blake.
Target Milestone: --- → mozilla1.9alpha6
Comment 8•18 years ago
|
||
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
Assignee | ||
Comment 9•18 years ago
|
||
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.
Status: NEW → ASSIGNED
Comment 10•18 years ago
|
||
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...
Assignee | ||
Comment 11•18 years ago
|
||
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•18 years ago
|
||
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.
Assignee | ||
Comment 13•18 years ago
|
||
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.
Attachment #270777 -
Flags: review?(jst)
Attachment #270777 -
Flags: review?(bzbarsky)
Updated•18 years ago
|
Attachment #270777 -
Flags: review?(jst) → review+
Assignee | ||
Updated•18 years ago
|
Attachment #270777 -
Flags: review?(bzbarsky) → superreview?(bzbarsky)
Assignee | ||
Comment 14•18 years ago
|
||
Comment on attachment 270777 [details] [diff] [review]
patch v1
I don't know if Boris is doing reviews right now...
Attachment #270777 -
Flags: superreview?(bzbarsky)
Attachment #270777 -
Flags: superreview?(brendan)
Attachment #270777 -
Flags: review?(bzbarsky)
Comment 15•18 years ago
|
||
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
Attachment #270777 -
Flags: superreview?(brendan) → superreview+
Assignee | ||
Comment 16•18 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 17•18 years ago
|
||
This might have caused a big leak regression.
RLk:240KB
Lk:4.12MB
Assignee | ||
Comment 18•18 years ago
|
||
(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•18 years ago
|
||
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...
Attachment #270777 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•18 years ago
|
||
This was next on the list, backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 21•17 years ago
|
||
I applied this patch to my build, it crashed at exit.
@NS_LogCOMPtrRelease_P
Because of the leak issue?
Assignee | ||
Comment 22•17 years ago
|
||
It's possible. I didn't run into anything like that. Do you know what object was being destroyed when it crashed?
Reporter | ||
Comment 23•17 years ago
|
||
stack attached.
Assignee | ||
Comment 24•17 years ago
|
||
That stack looks like bug 382350.
Priority: -- → P5
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.
mrbkap, are you ok with me relanding this?
This patch is updated to current trunk, carrying all flags forward.
Attachment #270777 -
Attachment is obsolete: true
Attachment #297104 -
Flags: superreview+
Attachment #297104 -
Flags: review+
Assignee | ||
Comment 28•17 years ago
|
||
bent, if you have the cycles to try to land this again, I'd really appreciate it.
Assignee | ||
Comment 29•17 years ago
|
||
Also, I never attached a patch that dealt with brendan's nit in comment 15, which should be picked before final checkin.
Relanded. Early results look good, so resolving. I'll keep an eye on the leak numbers today.
Status: REOPENED → RESOLVED
Closed: 18 years ago → 17 years ago
Resolution: --- → FIXED
Oh, and I fixed the nit from comment 15.
Comment 32•17 years ago
|
||
I think this may have caused bug 412598.
Comment 33•17 years ago
|
||
and maybe bug 413093
Comment 34•15 years ago
|
||
bug 418426 also appears to be a side-effect of this patch. Would appreciate help in resolving that bug too.
You need to log in
before you can comment on or make changes to this bug.
Description
•