148458 - Netscape Radio crashes using Real Player if another plugin is being installed in another window because scripting any plugin in other window after plugins.refresh(1) causes this crash when plugin has been unloaded - N70PR1 [@ NPPL3260.DLL - XPTC_Invoke...

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Peter Lubczynski]

This is the same problem just in simpler form as bug:
http://bugscape.netscape.com/show_bug.cgi?id=15482

Steps to reproduce:
1. Ensure RealPlayer is installed correctly -- if not locate and copy
nppl3260.dll and nppl3260.xpt to your plugins folder.

2. Visit http://radio.netscape.com Launch the player and listen to a station
3. In your plugins folder, do a 'touch *' to ensure we scan
4. In the other browser window, in the URL bar type: 
javascript:navigator.plugins.refresh(1);

5. Now try to control the Radio. Change the volume or station, for example.
6. Noice this crash:

NPPL3260! 6155a6a3()
XPTC_InvokeByIndex(nsISupports * 0x046767a4, unsigned int 3, unsigned int 2,
nsXPTCVariant * 0x0012e968) line 106
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 1994 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x032f4e38, JSObject * 0x03d2d910, unsigned int 1,
long * 0x03e85190, long * 0x0012ec18) line 1266 + 14 bytes
js_Invoke(JSContext * 0x032f4e38, unsigned int 1, unsigned int 0) line 788 + 23
bytes
js_Interpret(JSContext * 0x032f4e38, long * 0x0012f538) line 2743 + 15 bytes
js_Invoke(JSContext * 0x032f4e38, unsigned int 1, unsigned int 2) line 805 + 13
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x02a66c20,
nsXPCWrappedJS * 0x0319d4e0, unsigned short 3, const nsXPTMethodInfo *
0x01065d00, nsXPTCMiniVariant * 0x0012fa2c) line 1193 + 21 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x0319d4e0, unsigned short 3,
const nsXPTMethodInfo * 0x01065d00, nsXPTCMiniVariant * 0x0012fa2c) line 430
PrepareAndDispatch(nsXPTCStubBase * 0x0319d4e0, unsigned int 3, unsigned int *
0x0012fadc, unsigned int * 0x0012facc) line 115 + 31 bytes
SharedStub() line 139
nsXMLHttpRequest::RequestCompleted() line 970
nsXMLHttpRequest::OnStopRequest(nsXMLHttpRequest * const 0x03801918, nsIRequest
* 0x03ad5c68, nsISupports * 0x00000000, unsigned int 0) line 882
nsStreamListenerTee::OnStopRequest(nsStreamListenerTee * const 0x03057138,
nsIRequest * 0x03ad5c68, nsISupports * 0x00000000, unsigned int 0) line 66
nsHttpChannel::OnStopRequest(nsHttpChannel * const 0x03ad5c6c, nsIRequest *
0x0362367c, nsISupports * 0x00000000, unsigned int 0) line 2915
nsOnStopRequestEvent::HandleEvent() line 213
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x03ca5e7c) line 116
PL_HandleEvent(PLEvent * 0x03ca5e7c) line 596 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00b29e88) line 526 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x0012067a, unsigned int 49362, unsigned int 0,
long 11705992) line 1077 + 9 bytes

What I can see is going on is that the plugin instance is being stopped by the
host by the refresh command. Normally a reload of the page should start the
instance back up, but this does not happen because it's in another window. If I
hack around that problem, it still crashes. I think something is trying to
access the plugin in the unload handler or some mouse event.

Does anyone know if it would be possible to check for a valid plugin instance
before allowing the call in? Can XPConnect somehow be "reset"?

Comment 1

[shrirang khanzode]

moving over nominations from the bugscape bug to get adt eyeballs on this one. 
This one covers crashers for ipix and one for viewpoint too. so if this is 
fixed..we get 2 more crashers fixed along with this (hopefully)

Comment 2

[Jaime Rodriguez, Jr.]

what's the bugscape bug number?

Comment 3

[Steve Mellor]

I'm seeing this with 1.0 under linux (Slackware8, kernel 2.4.16)

 this: http://www.virtuetv.com/music/videos/ninjatunes/vector_4654_r_20.ram

causes the browser to exit

 this: http://www.unitedspacealliance.com/live/nasatv.htm

is OK (the plugin starts)


it was fine under 1.0rc3 (linux)

Comment 4

[shrirang khanzode]

jaime, it's these  bugs:

http://bugscape.netscape.com/show_bug.cgi?id=15482
http://bugzilla.mozilla.org/show_bug.cgi?id=148889

Comment 5

[Peter Lubczynski]

Attachment: patch v.1

Linux does not have a scriptable Real Player plus the testcase is to a
full-page plugin which are not even scriptable so I doubt Steve is seeing the
same problem. It could be bug 149230.

Here is a patch for this crash. It fixes it by causing the frame tree wherever
there is a running plugin to be reframed. This has the effect of creating a new
plugin instance (because plugins are tried to frames, see bug 90268) but not
causing any events to fire to Javascript. Please take a look and let me know
what you think. The patch is not optmized yet as I was crashing when I tried to
reframe just my object frame.

Comment 6

[av (gone)]

Peter, can you give a little more background, i.e. is frame reconstruction
something which happens here and there, what are other situations when we need
it, other than reloading pages? Would not just nuking the instance and reflowing
the frame help? Sorry for silly questions, I am trying to understand how costy
the approach is going to be. Would not it be more simple and straightforward
just to reload all open browser windows?

Comment 7

[Peter Lubczynski]

I tried doing a simple reload however I believe there is something in the onload
handler which is causing us to crash. Reloading the page isn't good because
script might execute and crash or the user may see unexpected results. 

What we really need to do is just reinstantiate the plugin. We don't really need
a new frame, just a new instance owner. However since it's so tightly bound to
the frame, might as well do the reframe which will give us what we want -- a new
instance owner with a new plugin instance. I think reframing the whole document,
possibly once for each instance on the page, could be overkill. Optimally, if it
just wouldn't crash, the best thing would be to target the reframe just to the
object frame. 

Comment 8

[Peter Lubczynski]

Attachment: patch v.2

Here's an updated patch with more comments. Radio even restarts automagically!
Please review.

The more I think about the reframe of the whole document, the more I think is
the right thing to do. We don't want to run any scripts when the page is
unloaded however we need layout to give us a chance to render tags which didn't
have the right installed plugin. For example, if an XPI script is finished
installing in one window, the plugin should be visible in the other window
where before there could have been text or a default plugin.

Comment 9

[av (gone)]

I guess the patch is not complete. There should be at least nsPluginHostImpl.h
in it.

Comment 10

[Jaime Rodriguez, Jr.]

Peter - What are the chnaces we'd have a patch and reviews by tomorrow? Pls add
you ETA to the Status Whiteboard.

Comment 11

[av (gone)]

Attachment: patch v.2.1

Missing file to the previous patch to make it complete.

Comment 12

[av (gone)]

Comment on attachment 87179 [details] [diff] [review]
patch v.2

Peter, this patch is either incomplete or requires more work. Although it does
fix the original crash it does not seem to address full-page cases. In my only
try I got a non-repaintable window with Flash, just like in the other but with
WMP.

As to the approach you are trying to take, it looks good to me -- reasonable
and solid.

Comment 13

[Peter Lubczynski]

aww...shucks....full-page not workin! 

Okay, got an idea to try out:
the nsIDocument returned from nsIPluginInstance owner is pretty much empty. It's
possible that when we create it in nsPluginViewer, that we can call
|nsIDocument::SetContainer| to hold our docshell (the container of
nsPluginViewer). We can then later get this out of the document and since we
have access to the docshell, we can simply reload.

Comment 14

[Peter Lubczynski]

Attachment: patch v.3

New patch:
-reload full-page plugins
-reframe embedded plugins

Please review.

Comment 15

[av (gone)]

Attachment: stack trace

With the patch applied I crash doing the following:

- open a page with simple embedded Flash
- open a new window with full page Flash
- touch a file in the Plugins folder to ensire rescan
- type javascript:navigator.plugins.refresh(1) in the latter window URL bar
- assertion fires with the above stack and then crashed (exits) on Ignore

Comment 16

[av (gone)]

False alarm. The crash does not seem to be related to the patch.

Comment 17

[Peter Lubczynski]

Perhaps a new bug needs to be filed on the newly discovered crash since it
happens with this patch.

Comment 18

[Peter Lubczynski]

um...I meant to say since it doesn't happen with this patch. 

Comment 19

[av (gone)]

You probably meant 'since it happened without the patch'

Comment 20

[av (gone)]

Comment on attachment 87578 [details] [diff] [review]
patch v.3

r=av

Comment 21

[Peter Lubczynski]

yeah, never mind what I said above, the crash happens even without the patch.
Bug 152640 has been opened for that issue.

Comment 22

[Jay Patel [:jay]]

Adding testcase, topcrash+ keywords and N70PR1 [@ NPPL3260.DLL -
XPTC_InvokeByIndex] to summary for tracking.

Comment 23

[Peter Lubczynski]

Attachment: patch v.4

updated patch based on Johnny's comments:
* Removed nsCOMPtr function arguments
* adjusted by using an sgo for missing interface methods on nsIDocument on
branch

works nicely on the branch now :)

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 89602 [details] [diff] [review]
patch v.4

+struct nsPluginDocReframeEvent: public PLEvent {
+  nsPluginDocReframeEvent (nsISupportsArray* aDocs);
+ 
+  void HandleEvent() {    
+    if (mDocs) {
+      PRUint32 c;
+      mDocs->Count(&c);

... tons of code...

Any real reason for this HandleEvent() to be inline?

... same method:

+	       nsCOMPtr<nsIDocShell> docShell;
+	       gso->GetDocShell(getter_AddRefs(docShell));
+	       if (docShell) {
+		 nsCOMPtr<nsIWebNavigation> webNav =
do_QueryInterface(docShell);

The above |if (docShell)| check is not needed, do_QueryInterface() is null
safe.

+		 if (webNav)
+		   webNav->Reload(nsIWebNavigation::LOAD_FLAGS_NONE);
+

- Further down:

>+          PL_InitEvent(ev, nsnull, (PLHandleEventProc) ::HandlePluginDocReframePLEvent, (PLDestroyEventProc) ::DestroyPluginDocReframePLEvent);
>+          eventQueue->PostEvent(ev);

Why the PLDestroyEventProc casts, they shouldn't be needed if the signatures
match.

- In nsPluginViewer.cpp:

+      nsCOMPtr<nsIInterfaceRequestor>
requestor(do_QueryInterface(mContainer));
+      if (requestor) {
+	 nsCOMPtr<nsIScriptGlobalObject> global;
+	 requestor->GetInterface(NS_GET_IID(nsIScriptGlobalObject),
+				 getter_AddRefs(global));

All the above can be replaced with:

       nsCOMPtr<nsIScriptGlobalObject> global(do_GetInterface(mContainer));

+	 if (global) {
...

sr=jst if you fix the above.

Comment 25

[Peter Lubczynski]

Attachment: patch v.5

updated patch to Jonny's comments.

In nsPluginViewer.cpp, I could not change to using:
nsCOMPtr<nsIScriptGlobalObject> global(do_GetInterface(mContainer));
The web shell is a nsIScriptGlobalObjectOwner. Should I get the gso out of
that?

Comment 26

[Peter Lubczynski]

Attachment: patch v.5.1

Never mind that last comment, |do_GetInterface| works fine.

Comment 27

[av (gone)]

Comment on attachment 90103 [details] [diff] [review]
patch v.5.1

r=av

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 90091 [details] [diff] [review]
patch v.5

nsCOMPtr<nsIScriptGlobalObject> global(do_GetInterface(mContainer));

That'll work, mContainer is a webshell, do_GetInterface() will first QI to
nsIInterfaceRequestor and then call GetInterface() on it, no need for you to do
that when do_GetInterface() can do it for you.

sr=jst if you make that change.

Comment 29

[Peter Lubczynski]

patch in trunk, marking FIXED and nominating

Comment 30

[shrirang khanzode]

verified on 0708 trunk. this is working great now, no crash seen. tried out my 
testcase from bug 15482

Comment 31

[scottputterman]

adding adt1.0.1+.  Please get drivers approval before checking into the branch.

Comment 32

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 33

[Peter Lubczynski]

patch in branch

Comment 34

[shrirang khanzode]

checked out the fix on branch (0716), looks good.

Comment 35

[greer]

Attachment: Stack and comments from N7.0 (Gecko1.0) data.

Peter, Shrirang,
I'm hesitant to open a bug that is marked verified fixed, but there are a
combined 145 crashes at the NPPL3260.DLL and nppl3260.dll signatures in the
Talkback data for N7.0 (Gecko1.0 2002082310) and 25 from the M110 release
(MozillaTrunk 2002082611). Stacks match and behavior seems to match. Can we
verify that this is a separate issue? Or does this bug need to be revisited?

Comment 36

[Peter Lubczynski]

This bug was only for the plugins.refresh(1) crash. Open new bugs other crashes.
Also see bug 158670.

Comment 37

[Peter Lubczynski]

The original summary for this bug was longer than 255 characters, and so it was truncated when Bugzilla was upgraded. The original summary was:

Netscape Radio crashes using Real Player if another plugin is being installed in another window because scripting any plugin in other window after plugins.refresh(1) causes this crash when plugin has been unloaded - N70PR1 [@ NPPL3260.DLL - XPTC_InvokeByIndex]