293987 - Mozilla cannot open PDF document via proxy and It crash after close the tab when it loaded

Status: RESOLVED FIXED

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

Description

[Leon Sha]

Launch Mozilla, browse to
http://mediacast.sun.com/share/leon/JDSR3GAG_8190919_d4.pdf. 
When you are using proxy, acroread appears to start (as mozilla plugin) but
nothing is displayed.
When you access the page directly or save this pdf to local file and open if,
the document successfully opens in mozilla's acroread plugin.

This bug only happened on linux and solaris, not on windows.

Comment 1

[Leon Sha]

The problem I found it here:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#6617

6603   mInstance->Stop();
6604   mInstance->Start();
..................................
6617         ((nsPluginNativeWindow*)window)->CallSetWindow(inst);

In ns4xPluginInstance::Stop, mozilla destoried mXtBin, but not window->ws_info.
So in ns4xPluginInstance::SetWindow mXtBin is null, but window->ws_info is not
null. This is not a correct situration.

This bug maybe related to bug 94895 and Bug 119494.

Comment 2

[Brendan Eich [:brendan]]

Thanks, Leon.  Giving to jst to track for 1.8b3.

/be

Comment 3

[Darin Fisher]

Leon, can you please capture a HTTP log?  Instructions here:
http://www.mozilla.org/projects/netlib/http/http-debugging.html

Thanks!

Comment 4

[Leon Sha]

Attachment: http log with proxy enabled

Comment 5

[Leon Sha]

Attachment: http log with proxy disabled

Comment 6

[Leon Sha]

Before I get these logs, I apply the temp patch for bug 246560.

Comment 7

[Asa Dotzler [:asa]]

Darin, can you see anything obvious in these logs?

Comment 8

[chris hofmann]

darin will look at as soon as he gets over the hump on updater and other stuff.

Comment 9

[Jessie Li]

This bug has more critical impact to users, steps to reproduce:
1. Start Mozilla.
2. Open a new tab and go to the following link:
http://java.sun.com/j2ee/1.4/docs/tutorial/doc/J2EETutorial.pdf
[BUG:the file can't display in browser]
3. Open another tab and go to http://www.sina.com.cn
[page load successfully]
4. Go back to the previous page and reload the page.
[PDF file displays correctly]
5. Close this tab either by right clicking the tab and selecting 'close tab' or
by clicking the X at the right upper corner.
[BUG:Mozilla crash; if you did close www.sina.com.cn tab instead of PDF tab, go
back to PDF tab, the PDF contents disappears.]

But this bug didn't happen to all the PDF files. It only happens to a small part
of PDF files. Unfortunately, we have not find which kind of PDF files cause this
bug. 
We tried 193 PDF files, only 5 had this problem. Test files can be found in the
following link:
http://acroeng.adobe.com/file_elements.html

Comment 10

[Darin Fisher]

Thanks for the extra details on reproducing this bug.  I plan to fix this for
Firefox 1.1 beta.

Comment 11

[RNicoletto]

WFM 

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050704
Firefox/1.0+ (stipe)

Windows XP SP2 + Acrobat Reader 7.02

Comment 12

[Leon Sha]

(In reply to comment #11)
> WFM 
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050704
> Firefox/1.0+ (stipe)
> 
> Windows XP SP2 + Acrobat Reader 7.02

This bug only happened in linux/unix.

Comment 13

[Asa Dotzler [:asa]]

Darin, do you think that a fix here will be isolated and low-risk?

Comment 14

[Darin Fisher]

I have no idea.  I haven't had time to investigate this bug yet.  I could
certainly use help.

Comment 15

[Asa Dotzler [:asa]]

Do we have a stacktrace for the crash? I don't think this is going to make 1.5
without someone coming up with a very safe patch in the next few days. If that
happens, please request approval at the patch flag.

Comment 16

[Leon Sha]

Attachment: Patch

Comment 17

[Darin Fisher]

Leon: can you please explain why this patch fixes the bug?

Comment 18

[Leon Sha]

(In reply to comment #17)
> Leon: can you please explain why this patch fixes the bug?
> 
http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/src/nsPluginHostImpl.cpp#6662
After mInstance->Stop(), mXtBin has been destoried.
The fact is we does not really want to destory mXtbin here.
If mXtbin needs to be destoried, mInstance->Destory() should be called. So I just move the destory mXtbin part to mInstance->Destory().

Comment 19

[Brendan Eich [:brendan]]

I think Leon has a promising patch here.  jst should sr.

/be

Comment 20

[Darin Fisher]

I really don't understand this code at all.  What is mXtBin?  ns4xPluginInstance::Destroy says that destruction happens in ::Stop.  Was the original author just wrong about that?  Is there some better way to destroy mXtBin in Stop that does not cause this bug?  I really don't know this code well enough to say with any certainty that "yeah, this is the proper fix."

Comment 21

[Christian :Biesinger (don't email me, ping me on IRC)]

this patch is only for the second half of the summary, right?

Comment 22

[Leon Sha]

(In reply to comment #20)
> I really don't understand this code at all.  What is mXtBin? 
> ns4xPluginInstance::Destroy says that destruction happens in ::Stop.  Was the
> original author just wrong about that?  Is there some better way to destroy
> mXtBin in Stop that does not cause this bug?  I really don't know this code

Maybe we can do this. Just not use mInstance->Stop() here.
In nsPluginStreamListenerPeer::ServeStreamAsFile we just need to clean up the streams, so we give it a method to clean up the streams and do not use mInstance->Stop().

> well enough to say with any certainty that "yeah, this is the proper fix."
> 

Comment 23

[Leon Sha]

(In reply to comment #21)
> this patch is only for the second half of the summary, right?
> 
I don't quite understand you meaning.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #23)
> I don't quite understand you meaning.

It doesn't fix "Mozilla cannot open PDF document via proxy", does it?

Comment 25

[Asa Dotzler [:asa]]

we should release note the save and open in standalone viewer workaround.

Comment 26

[Leon Sha]

Attachment: Patch v2

This is a more reasonable patch. 
Since mXtBin is destroied, I create a new one here.

Comment 27

[Darin Fisher]

Comment on attachment 207056 [details] [diff] [review]
Patch v2

>Index: nsPluginHostImpl.cpp

>+#if defined (MOZ_WIDGET_GTK) || defined (MOZ_WIDGET_GTK2)
>+      // Should call GetPluginPort() here.

Why aren't we calling GetPluginPort() then?

Aagin, I'm not very comfortable reviewing this code because I
don't know what it is doing.  Isn't there someone who knows
these APIs better?

Comment 28

[Brendan Eich [:brendan]]

blizzard, caillon, maybe bryner?  shaver, any thoughts?

/be

Comment 29

[Leon Sha]

(In reply to comment #27)
> (From update of attachment 207056 [details] [diff] [review] [edit])
> >Index: nsPluginHostImpl.cpp
> 
> >+#if defined (MOZ_WIDGET_GTK) || defined (MOZ_WIDGET_GTK2)
> >+      // Should call GetPluginPort() here.
> 
> Why aren't we calling GetPluginPort() then?
> 

GetPluginPort() is not an interface of nsIPluginInstanceOwner.
It is just a function of nsPluginInstanceOwner.

> Aagin, I'm not very comfortable reviewing this code because I
> don't know what it is doing.  Isn't there someone who knows
> these APIs better?

I agree with you. It is better to have someone familiar with this module to review this. 

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 207056 [details] [diff] [review]
Patch v2

sr=shaver

Comment 31

[Leon Sha]

Checking in base/src/ns4xPluginInstance.cpp;
/cvsroot/mozilla/modules/plugin/base/src/ns4xPluginInstance.cpp,v  <--  ns4xPluginInstance.cpp
new revision: 1.132; previous revision: 1.131
done
Checking in base/src/nsPluginHostImpl.cpp;
/cvsroot/mozilla/modules/plugin/base/src/nsPluginHostImpl.cpp,v  <--  nsPluginHostImpl.cpp
new revision: 1.550; previous revision: 1.549
done