80794 - XPCDOM changes broke nsPluginInstancePeerImpl::GetJSWindow

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[sean echevarria]

The recent XPCDOM changes broke nsPluginInstancePeerImpl::GetJSWindow.  This 
method does a QI on an nsIScriptGlobalObject to get an nsIScriptObjectOwner.  
The changes swapped out nsIScriptObjectOwner for nsIDOMJSWindow in 
GlobalWindowImpl (http://bonsai.mozilla.org/cvsview2.cgi?
diff_mode=context&whitespace_mode=show&file=nsGlobalWindow.cpp&root=/cvsroot&sub
dir=mozilla/dom/src/base&command=DIFF_FRAMESET&rev1=1.395&rev2=1.396).

Comment 1

[sean echevarria]

The QI on global for nsIScriptObjectOwner at 
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginInstancePe
er.cpp#854 fails and window comes back with a null raw pointer.

Updating keywords.  This affects all sites that use the Beatnik Player plugin.

Comment 2

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 3

[Johnny Stenback (:jst)]

Sean, can you test the attached patch?

Comment 4

[sean echevarria]

tested the patch and it addresses the problem.  I'll attach a revised patch
tomorrow that removes the instantiation of the JVM manager since it isn't used
(it was unused to begin with, likewise with the GetJSContext method).

Sadly for me though, some xpconnect interfaces changed and the Beatnik plugin
will crash post May 8 builds when it makes calls through vtables that are not
what they used to be. :(

Comment 5

[Johnny Stenback (:jst)]

Sean, thanks for testing this, and for actually looking at what the code is
doing :-)

What interface are you calling into (and what object?) when you see the crash?

Comment 6

[sean echevarria]

Attachment: updated patch - removes unnecessary instantiation of nsIJVMManager service

Comment 7

[sean echevarria]

Attachment: updated patch - removes unnecessary instantiation of nsIJVMManager service (-wu diff))

Comment 8

[sean echevarria]

>What interface are you calling into (and what object?) when you see the crash?

I'm still looking into the changes but this is probably about it:

1. I use nsIXPConnect to call GetCurrentNativeCallContext.  
nsIXPConnect got a new method GetWrappedNativeOfNativeObject that was added 
before GetCurrentNativeCallContext.

2. GetCurrentNativeCallContext is used to get a nsIXPCNativeCallContext*.
The nsIXPCNativeCallContext* is used to call GetCallee, GetJSContext, GetArgc, 
and GetArgvPtr.
nsIXPCNativeCallContext got two new methods (GetCalleeInterface and 
GetCalleeClassInfo) that were added before GetJSContext, GetArgc, and 
GetArgvPtr.

3. Something changed with nsIXPCNativeCallContext::GetCallee().
I used to successfully do this:
  // pCc is the nsIXPCNativeCallContext* I got from GetCurrentNativeCallContext
  nsISupports *pCallee = nsnull;
  rv = pCc->GetCallee(&pCallee);
  if (NS_SUCCEEDED(rv) && pCallee == (IBeatnikPlugin*)this)
Now the comparison doesn't work - have to QI pCallee and this to the same 
interface (admittedly, should have done this to begin with).

4. And nsIScriptSecurityManager changed rather drastically (new methods and new 
base interface).

Comment 9

[Johnny Stenback (:jst)]

sean, not sure there's a whole lot we can do to make that incompatibility go
away w/o major changes, please file that problem as a separate bug against jband.

Comment 10

[sean echevarria]

I guess I made this messy by making changes to the patch.
Do we need dual r=?
r=[e-mail] for the initial patch.
Johnny, can you put your stamp on the modifications I made?

Comment 11

[David Hyatt]

sr=hyatt

Comment 12

[Johnny Stenback (:jst)]

Thanks for the patch and the review, patch checked in.

Comment 13

[sean echevarria]

vrfy'd