50547 - Browser stops responding when url is loaded and back button is pressed

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect, P2)

Description

[shrirang khanzode]

Build: 2000082804m18 windows

Go to the above url
After the page loads, press the BACK button
Observe that the url bar shows the previous url from the history but the page 
does not load and the browser goes in a limbo. Menus can be clicked but nothing 
functions.
Browser needs a  restart to be working again.

Comment 1

[shrirang khanzode]

nominating for nsbeta3 since this is a majorproblem and seen on lot of applet 
pages.

Comment 2

[edburns]

reassign to edburns.

Comment 3

[edburns]

Shirang, can you please post the testcase as an attachment so non-netscape 
employees can see it?

thanks,

Ed

Comment 4

[shrirang khanzode]

oops..sorry about that. Updated the url.

Comment 5

[edburns]

*** Bug 49438 has been marked as a duplicate of this bug. ***

Comment 6

[edburns]

*** Bug 21757 has been marked as a duplicate of this bug. ***

Comment 7

[edburns]

*** Bug 32129 has been marked as a duplicate of this bug. ***

Comment 8

[edburns]

Attachment: cvs diff -u of fix for this bug, first iteration.

Comment 9

[edburns]

Attachment: cvs diff of fix for this bug, first iteration

Comment 10

[edburns]

Attachment: tar.gz of the files in the fix for this bug, first iteration

Comment 11

[edburns]

Posted this to [e-mail]

I know this isn't the place to solicit code review, but this bug fix is a 
substantial change in strategy for plugins.

Summary: 

Looking for approval to check in changes to the plugin lifecycle...make it be 
the 4.x plugin lifecycle.

Detail:

This fix was requested by Stanley Ho of the Sun Java Plugin Team.
Stanley floated a proposal some time ago on [e-mail],
n.p.m.{oji,java,plugins} about changing the mozilla plugin lifecycle to
be back to the ns4.x plugin lifecycle in the following respect.

Stanley's proposal:

When you visit a page with a mozilla (not 4.x) plugin:

  1. the plugin is loaded

  2. nsIPluginInstance::Initialize() is called

  3. nsIPluginInstance::Start() is called

When you leave the page with a running mozilla (not 4.x) plugin:

  1. nsIPluginInstance::Stop() is called

  2. nsIPluginInstance::Destroy() is called

  3. nsIPluginInstance::SetWindow(nsnull) is called

The bug fix for 50547 implements the above behavior.  Here's what I did:

  Remove method nsPluginHostImpl::FindStoppedPluginForURL().

  Remove method nsPluginHostImpl::AddInstanceToActiveList().

  Remove method nsIPluginHost::StopPluginInstance().

  Remove ivar nsActivePluginList mActivePluginList.

  Remove class nsActivePlugin.

  Remove class nsActivePluginList.

  Remove macro MAX_NUMBER_OF_STOPPED_PLUGINS.

Files in this patch:

M modules/plugin/nglsrc/nsIPluginHost.h
M modules/plugin/nglsrc/nsPluginHostImpl.cpp
M modules/plugin/nglsrc/nsPluginHostImpl.h
M modules/plugin/nglsrc/nsPluginViewer.cpp
M layout/html/base/src/nsObjectFrame.cpp

Please visit the bug to see the patches:

http://bugzilla.mozilla.org/show_bug.cgi?id=50547

Comment 12

[edburns]

Pending approval, looking at ETA 11 Sept 00.

Comment 13

[edburns]

Attachment: cvs diff -u of fix for this bug, second iteration (brought up to tip)

Comment 14

[edburns]

Attachment: cvs diff of fix for this bug, second iteration (brought up to tip)

Comment 15

[edburns]

Attachment: tar.gz of fix for this bug, second iteration (brought up to tip)

Comment 16

[sean echevarria]

1. A background question.  How does getting rid of the plugin cache affect the 
applets bug?  Specifically, what is currently cached: applet instances, the 
java plugin or both?

2. nsPluginViewer.cpp - should remove these lines also:
     nsCOMPtr<nsIPluginHost> host;
     host = do_GetService(kCPluginManagerCID);

3. As best as I can tell, calling SetWindow() after a plugin has been     
Destroy()'d is not 4x behavior and doesn't seem like a safe thing to start 
doing.

Comment 17

[edburns]

Well, getting rid of the plugins cache is a necessary effect of causing the 
browser to send Destroy() to the plugin on page switch.  Sending Destroy() on 
page switch is what this is about, and we can't keep an entry in the "running 
plugins" list for plugins that have been destroyed.

Comment 18

[sean echevarria]

So the problem is only that the java plugin is cached?

If so, an alternative is to add another java special case like this:
    (PL_strcasecmp(aMimeType, "application/x-java-vm") != 0 && 
     PL_strcasecmp(aMimeType, "application/x-java-applet") != 0))

before the call to AddInstanceToActiveList() in 
nsPluginHostImpl::SetUpPluginInstance().

Then in the java plugin Stop() method, call Destroy() directly (since it won't 
come from the nsActivePlugin dtor).

Comment 19

[sean echevarria]

Or even better, add a method to nsIPluginInstance like:
PR_BOOL OkToCache();

And call it inside of AddInstanceToActiveList().

So that plugins instances can govern their own destiny.

Comment 20

[drapeau]

Updating status: fixing this will resolve critical crash issue for Java.

Comment 21

[edburns]

Adding CCs.

I'm about to attach the fix for this bug discussed in the conference call today.

Here's my cvs log message for this checkin.

Summary:

Looking for approval to check in changes that allow plugins to indicate:

1. Whether or not they want to allow the browser to cache their
instance.  Default is yes, do allow the browser to cache their instance.

2. If they answer no to 1, that is, no the plugin does not want the
browser to cache their instance, do you want the shutdown calls to be:

a. 

          inst->SetWindow(nsnull);
          inst->Stop();
          inst->Destroy();


b.

          inst->Stop();
          inst->Destroy();
          inst->SetWindow(nsnull);

a. is the default.

Please visit the bug to see the patches:

http://bugzilla.mozilla.org/show_bug.cgi?id=50547

Detail:

This fix was requested by Stanley Ho of the Sun Java Plugin Team.  A
conference call between Eric Krock, Andrei Volkov, Sun, Adobe and other
plugin vendors was used to agree on the above solution.

M modules/plugin/public/nsplugindefs.h
M modules/plugin/nglsrc/nsPluginHostImpl.cpp
M layout/html/base/src/nsObjectFrame.cpp

Comment 22

[edburns]

Attachment: cvs diff -u of fix for this bug, third iteration.

Comment 23

[edburns]

Attachment: cvs diff of fix for this bug, third iteration.

Comment 24

[edburns]

Attachment: tar.gz of all the files in the fix for this bug, third iteration.

Comment 25

[edburns]

PLEASE note that a new build of the java plugin that honors these changes is 
required to observe the fix for this bug.

Comment 26

[av (gone)]

Ed, should not it be 
   if (!doCache) 
in place of your 
   if (doCache)
in nsObjectFrame.cpp?

Comment 27

[edburns]

Sean, yes, definately PR_TRUE.  

Sean, yes, should be !doCache.  I'm going to repost diffs and tar.gz.

Comment 28

[edburns]

Attachment: cvs diff -u of fix for this bug, fourth iteration.

Comment 29

[edburns]

Attachment: cvs diff of fix for this bug, fourth iteration

Comment 30

[edburns]

Attachment: tar.gz of fix for this bug, fourth iteration

Comment 31

[edburns]

Yes, Sean already caught that.  I've fixed it in the fourth iteration patches.

Comment 32

[sean echevarria]

Looks like the solution everyone agreed to.

I know we didn't discuss this explicitly (because noone thought of it), but I 
think we also need to modify nsPluginViewer.cpp:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/nglsrc/nsPluginViewer.cpp
#754

The pluginInstanceOwner assumes that a plugin instance is cached.  No error is 
returned from StopPluginInstance if the plugin is not found in the cache and 
destroy will never be called on it (if the plugin instance opted out of the 
cache).

Comment 33

[Brendan Eich [:brendan]]

a=[e-mail] if sean's comment in the bug is an r= (is it?  What bug is 
on file for fixing nsPluginViewer.cpp?).

/be

Comment 34

[sean echevarria]

There won't be anything wrong with nsPluginViewer.cpp until the attached patch 
goes in.  It needs to be aware of the plugin lifecycle change this patch 
addresses for nsObjectFrame.cpp so that plugins can be destroyed properly (if 
they opted out of the cache - it currently works correctly if plugin instances 
are in fact cached).  

nsPluginViewer.cpp is used by full-page plugin instantiations.  Full-page 
instantiations occur when you click on a file whose mimetype is handled by an 
installed plugin (no html file involved).  The code touched in nsObjectFrame is 
not run in the case of full-page plugins.

Comment 35

[edburns]

Sean, I see, I'll make the change in nsPluginViewer as well.  I'll re-post the 
diffs.

Comment 36

[edburns]

Sean, nsPluginViewer's Destroy is a no-op.
Are you on IRC or AIM or something?

Comment 37

[sean echevarria]

Sorry - was out to lunch.

Regarding the Destroy() impl, correct.  But the line of importance is:
      host->StopPluginInstance(mInstance);
in the pluginInstanceOwner destructor.

That call relies on the instance being in the cache (in which case Destroy() is 
called when the plugin is removed from the cache).  If the plugin is not in the 
cache, then we need to call Destroy() on the instance after the Stop() instead 
of calling StopPluginInstance().  

Comment 38

[edburns]

For the record, make it so the beginning of pluginInstanceOwner :: 
~pluginInstanceOwner() does the same thing as nsObjectFrame::Destroy().

Comment 39

[edburns]

Attachment: cvs diff -u of fix for this bug, fifth iteration.

Comment 40

[edburns]

Attachment: tar.gz of all the files in the fix for this bug, fifth iteration.

Comment 41

[sean echevarria]

The only thing about the nsPluginViewer.cpp change is that this block could be 
an else block after the if (!doCache):
  nsCOMPtr<nsIPluginHost> host;
  host = do_GetService(kCPluginManagerCID);
  if(host)
    host->StopPluginInstance(mInstance);

As things are now, nothing bad happens.  The StopPluginInstance call will just 
fail.

r=sean
tested on WinNT with 4x adobe acrobat in full-page mode
tested with xpcom plugin with and without cache option in embed mode

Comment 42

[stanley.ho]

*** Bug 48050 has been marked as a duplicate of this bug. ***

Comment 43

[Phil Peterson]

PDT thinks this bug is a P2 because Java is only installed if you choose
"Complete". Most users will install "Typical" and thus won't see this bug.

Comment 44

[edburns]

This one is fixed.  However, you'll need a version of the JavaPlugin more 
recent than 18 Sept to verify it.

Comment 45

[shrirang khanzode]

Java plugin info :Java(TM) Plug-in: Version 1.3.0_01
                  Using JRE version 1.3.0_01-beta Java HotSpot(TM) Client VM
                  
Verified this is working fine on windows commercial br build 2000092908.Marking 
as such.