366724 - Crash in "vtable for XPCWrappedNativeScope" if setCommandContext() called with a GCable object

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Andreas Wuest]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1

If a "@mozilla.org/embedcomp/base-command-controller;1" is initialised with setCommandContext(param) and param != null, then executing IsCommandEnabled() on a registered command crashes in vtable for XPCWrappedNativeScope (see testcase for reproduction and backtrace).

Reproducible: Always

Steps to Reproduce:
1. Instantiate new "@mozilla.org/embedcomp/base-command-controller;1"
2. setCommandContext() with a parameter != null
3. register a command
4. call IsCommandEnabled
Actual Results:  
Crash in "vtable for XPCWrappedNativeScope"

Expected Results:  
No crash.

This happens with Firefox 2.0.0.1 as well as 1.5.

Comment 1

[Andreas Wuest]

Attachment: gdb backtrace

Comment 2

[Andreas Wuest]

Attachment: testcase

If setCommandContext(null) is called instead of setCommandContext(Container), then it does not crash.

(If somebody knows how to run this code outside of a priviledged chrome URI, please tell me so.)

Comment 3

[timeless]

Attachment: hold pointers properly?

you could have at least cc'd neil...

http://www.mozilla.org/projects/security/components/signed-scripts.html kinda explains how to do this w/o chrome.

Comment 4

[[e-mail]]

Comment on attachment 251262 [details] [diff] [review]
hold pointers properly?

Do any existing consumers expect this to hold a weak pointer without using a weak reference? (Note that you can avoid the crash in existing releases by implementing QueryInterface with nsISupportsWeakReference et. al.)

Comment 5

[Andreas Wuest]

(In reply to comment #3)
> Created an attachment (id=251262) [details]
> hold pointers properly?
> 
> you could have at least cc'd neil...

Sorry, I forgot that. I've tried to track him down on IRC instead.

> http://www.mozilla.org/projects/security/components/signed-scripts.html kinda
> explains how to do this w/o chrome.

Thanks! :)

Comment 6

[Boris Zbarsky [:bzbarsky]]

The interface documentation says:

 61    * @param aCommandContext  the context passed to commands.
 62    *                        Note that this is *not* addreffed by the
 63    *                        controller, and so needs to outlive it,
 64    *                        or be nulled out.

We could modify that to mention the nsWeakPtr thing if we want.

We could also change the API, but then we need to worry about leaks in existing code that uses the interface, no?

Comment 7

[timeless]

i don't like the idea of changing the interface.

is it possible for us to QI to determine if the object is a GC able object?

I'm pretty sure i can do that for js, i'm not sure how to do it for java.

Comment 8

[[e-mail]]

Comment on attachment 251262 [details] [diff] [review]
hold pointers properly?

r- as this change of contract has not been justified.

Comment 9

[timeless]

Attachment: testcase

this can be run using xpcshell and has no fluff.

Comment 10

[timeless]

Attachment: compare against known gcable objects

reporter: can you test this? :)

I haven't tested the Java and Python code, and I'd like to move the new files and header into xpcom/ since it should be something that xpcom provides somehow.

Design: each known annoying case (currently to my knowledge these are not registered with xpcom, which is sorta unfortunate) is given a fake interface file which enables me to use standard do_QI magic (cleaner from my perspective). 
If any of these annoying cases decide to multiply, then each new case would currently become a new file (with a new number, 2, 3 ...) and would be added as another 2 lines in the function.

Callers simply pass the object they don't trust to the function and are given NS_OK if it's safe to use or an error they can return to their callers when the object is unsafe.

A more scalable version of this would be a service which has two methods:
void registerGCableInterface(nsIID);
void isGCableObject(nsISupports);

where javascript, java, python, and friends would be expected to call the service and register their flavors.

Comment 11

[[e-mail]]

Better still, demand that all scriptable objects support a given interface?

Comment 12

[timeless]

that'd sure make our life easier....

Comment 13

[[e-mail]]

Can we get the best of both worlds by using the cycle collector?

Comment 14

[timeless]

i don't know anything about the cycle collector....

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 252470 [details] [diff] [review]
compare against known gcable objects

Almost certainly out of date and I'm no longer up on the code this patch touches.

Comment 16

[Andreas Wuest]

Comment on attachment 252470 [details] [diff] [review]
compare against known gcable objects

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

Is this approach still current?

Comment 17

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.