90959 - Plugins may not work at all on less than OS 9.1

Status: VERIFIED FIXED

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

Description

[Tracy Walker [:tracy]]

Seen on recent builds, trunk and branch.

-goto about:plugins to be certain that the flash player is present.

-goto the listed site to view a flash page. click on flash version

expected results: An animated Flash page should open and be in motion. 

tested results: the window where the Flash animation is supposed to view is 
blank.

Comment 1

[Alvaro Munoz-Aycuens]

On Mozilla/5.0 (Macintosh; U; PPC; en-US; rv:0.9.2+) Gecko/20010713 with MacOS
8.5 about:plugins shows the flash plugin loaded but when you go to any flash
site the plugin doesn't starts the animation saying that does not have a plugin
registered.

Comment 2

[Peter Lubczynski]

Investigating.......this is a regression as it works on my debug build from last
week but fails in this morning's bits from sweetlou. I just nuked my tree and am
starting a fresh pull/build of the branch.

Not all flash pages are effected. http://www.hardrockhotel.com worked for me. It
could be HTML related but that still doesn't explain why it worked in last
week's builds. The problem here is that the default plugin gets activated
instead of flash but Flash is indeed picked up in About:plugis, it's there and
works on other pages. 

Tested on the same build on Win32 and it works flawlessly.

Comment 3

[Brian Nesse (gone)]

I went to http://www.disney.com. It totally failed to bring up the flash
animation. I did an about:plugins to insure Flash was installed. Set some
breakpoints, went back to disney... and it loaded and ran. Now it runs the Flash
animation everytime I go there...even after quitting and re-launching... wierd.

Comment 4

[Peter Lubczynski]

Brian,
I'm still building but try clearing your disk cache. Or, another weird thing I
saw is that that going to about:plugins causes a plugins.refresh() which changes
things.  What happens if you go straight there?

Comment 5

[Brian Nesse (gone)]

I'll try the disk cache. After re-launching, I went straight there and it worked
correctly.

I just went from one page (disney main) to another (sub page) and got the "you
need to get the flash plugin" message. After that, no more flash...

It does appear that either the default plugin is taking precedence, or the
mimetype test is "missing" sometimes...

Comment 6

[Peter Lubczynski]

Could this be related to bug 90152? The patch in that bug is around code that
could effect this.

Comment 7

[Brian Nesse (gone)]

Clearing disk cache doesn't seem to have any effect. Currently, going to disney
from product launch, I get the functional page, click on any link, get broken
plugin image, go back to main page, get dialog that I don't have the plugin
finder. No more Flash until I quit and relaunch.

It's almost like the mimetypes get removed from the list after the first time
they are accessed.

Comment 8

[Brian Nesse (gone)]

I applied the patch from bug 90152 with no effect.

Comment 9

[Peter Lubczynski]

Just tried on Chris Petersen's machine with similar results:

1) Launch browser
2) Visiting http://www.hardrockhotelcom works
3) Then go to http://www.disney.com and it doesn't

Reverse #2 and #3 above and the second Flash page you visit doesn't work.

I'm <still> building but you can visit this page:
http://praline.netscape.com/plugins/manager3.html
That page goes through the navigator.plugins[] array without first doing a
plugins.refresh()

Hm...looing at CVS log, I'm now maybe suspecting my (1.275) checkin to
nsPluginHostImpl.cpp to make mime type comparison case-insensitive.

Comment 10

[Brian Nesse (gone)]

Hmm, those changes look pretty safe. I think I'd be more suspicious of the
LoadPlugin changes you made (don't remember the bug #, just recall thinking that
that code was pretty touchy and prone to breakage.)

Comment 11

[Brian Nesse (gone)]

It looks like the problem starts in nsObjectFrame::Reflow() where we we seem to
be falling into the code commented "Otherwise we're either an ActiveX control or
an internal widget..."

IsPluginEnabledForType is getting called with a mimetype of either
"application/oleobject" or "application/x-oleobject".

Comment 12

[Peter Lubczynski]

Brian,

I find the problem much deeper than that. If you set a breakpoint in
nsPluginHostImpl::GetPluginFactory(), you'll notice that about halfway through
we try to call nsPluginFile.LoadPlugin() which in turn causes PR_LoadLibrary to
be called in nsPluginDirMac.cpp.

The problem seems to be that PR_LoadLibrary() fails. Now for the hard part, why
does this fail?

Comment 13

[Brian Nesse (gone)]

Yeah, I had discovered that the oleobject thing was a red herring. It just
happened to be the first thing I came across that seemed odd.

Comment 14

[Brian Nesse (gone)]

It has the wrong path. I'm currently trying to open the library
"...mozilla:dist:viewer_debug:Shockwave Flash NP-PPC". Shockwave is in my
internet plugins folder.

Comment 15

[Peter Lubczynski]

Hm....I don't think that's it as I my System Folder:Internet Plug-ins folder is
empty and it still fails in PR_LoadLibrary with the correct path to the correct
plugin.

Clearing the regression keyword as I think you may have exposed this bug when
you fixed the nsITimer. That was probably broken for a long time and I bet
normal shutdown as on Windows was never being called on the Mac. We are probably
doing something wrong there, but I verify that TryUnloadPlugin() is being
called. In there we call Shutdown() on the plugin and also call
PR_UnLoadLibrary(). Btw, comenting out the library unload does not fix this.

Comment 16

[Brian Nesse (gone)]

You didn't not what your path is, but notice that mine would be wrong even if I
had the plugin in the application folder... there is no "Plug-ins" in that path
(or "Components" for that matter...

Comment 17

[Brian Nesse (gone)]

That should have said "You didn't note". You do appear to be correct about the
timer changes uncovering this... If you comment out the mPluginTimer = nsnull;
line in CancelTimer() the problem does go away. Something we are doing in the
nsPluginInstanceOwner destructor is causing this.

Comment 18

[Brian Nesse (gone)]

I'm on to something... In nsPluginHostImpl::StopPluginInstance() (line 4648) the
code checks to see if it is a "oldschool" (4.x) plugin. Right below this is a
check that does:

if (!doCache || oldSchool)
changing this to
if (!doCache || !oldSchool)
fixes the problem... But this logic wrong, or is it just another symptom?

Comment 19

[Peter Lubczynski]

I think that may just be a byproduct. If you change that, Shutdown() on the
plugin won't ever be called until we exit the browser. According to the spec,
it's supposed to be called when the last instance is going away.

Comment 20

[Peter Lubczynski]

Ahh, Brian, you were totally right about the path being incorrect....

Taking a closer look, it does indeed look like the call to PR_LoadLibrary() is
missing the "plug-ins" folder in the path. It goes straight from viewer_debug to
Shockwave. Looks like pluginTag->mFileName is in correct, as far as I can tell.

I think the problem may be in nsPluginDirMac.cpp, I'll take another peek tonight.

Comment 21

[Peter Lubczynski]

Nope, I see the problem now. I knew this was going to bite us sooner or later
and I guess fixing the leaking nsITimer exposed the problem.

On Windows (and on others I think), pluginTag->mFileName stores a full path to
the filename of the plugin. On, Mac this does exactly what it says, it stores
JUST the  filename. So, after we've gotten this plugin once, we save the
filename so that later we don't have to find it. I think
ns4xPlugin::SetPluginRefNum() may do that, however, I think it would require a
big hack in nsPluginHostImpl::GetPluginFactory() to use it. I think it would be
easier to fix GetPluginInfo() in nsPluginsDirMac.cpp to fully resolve fFilename.

Comment 22

[Peter Lubczynski]

Attachment: patch to save full paths on Mac v.1

Comment 23

[Peter Lubczynski]

On the flash testcases I tried tonight, that patch seems to fix it by using the
full path. Since the code is already expecting this on other platforms, it seems
to work. I did have some problems with Quicktime.

One interesting note though, I was crashing on the first call to
::CloseResFile().  It's probably not a good idea, but I just comented it out for
now.

Comment 24

[Peter Lubczynski]

Attachment: better patch v.2

Comment 25

[Peter Lubczynski]

I think that last patch should do the trick and should be very low risk.
Quicktime and Flash both work and keep working.

Comment 26

[av (gone)]

Peter, what is this mFullPath -- does it contain the leaf name as well? Also, 
haven't you miss one of the nsPluginTag constructors to initialize this new 
member?

Comment 27

[lchiang]

This looks pretty serious.  I will email PDT for PDT+ with a cc: to peter and
aruner.

Comment 28

[Brian Nesse (gone)]

Removing the CloseResFile call will cause us to keep the resource fork of the
plugin open for the lifetime of the Mozilla instance. This would be bad. That
close corresponds to the FSpOpenResFile in the constructor. The other open/close
pair is in the plugin directory scanning code which only happens at startup.

Oddly enough, I didn't have any trouble related to the CloseResFile call you
removed. There must be something else at play here...

If you are going to initialize mFullPath to nsnull in the third constructor, you
need to validate it before using it in GetPluginFactory.

You are going to leak mFullPath in FreePluginInfo.

Comment 29

[av (gone)]

Is there a good justification for having both mFileName and mFullPath? Why not 
just to add the path to mFileName and keep a parity among platforms?

Comment 30

[Brian Nesse (gone)]

Can't do it that way. Much of the underlying code on the mac expects to get just
the leaf name.

Comment 31

[Brian Nesse (gone)]

A better way to state that would be:

Much of the underlying Mac code expects just the file name. It could be changed,
but it would probably have far reaching consequences (based on what I saw when I
looked).

Comment 32

[Peter Lubczynski]

Brian is right, we can't make this to parity with the otherplatforms because the
Mac expects just the leaf name. Some plugins have problems.

So the ::CloseResFile call isn't giving you problems? Hm...I'll try it again and
also fix up FreePluginInfo so not to leak a mFullPath.

Lisa, yes, I think this is a stop-ship bug and would like PDT+ approval.

Comment 33

[Peter Lubczynski]

Attachment: patch addressing Brian's comments v.3

Comment 34

[lchiang]

PDT+ per PDT email for branch.  Trunk checkins do not need PDT approval.

Comment 35

[Brian Nesse (gone)]

Changes look good... I'm concerned about one thing though. Back to that third
constructor again...

That is the constructor that is used by LoadXPCOMPlugin(). It looks to me like
this patch will completely disable XPCOM plugins because mFullPath will be null
and therefore GetPluginFactory will always return NS_ERROR_FAILURE.

Either the filename should be used here (on the assumption that it worked
before) or the full path param should be passed to the constructor so it can be
set. (But then you won't have to check for null anymore :).)

Comment 36

[Peter Lubczynski]

Attachment: new patch, v.4

Comment 37

[Peter Lubczynski]

Okay, v.4 now takes care of the XPCOM case. It actually turns out that XPCOM
plugins were doing this correctly anyway and I confirm that the filename being
passed to the nsPluginTag constructor is the full and correct path to the plugin
(in the components directory). Since the filename is already being set
correctly, I simply set mFullPath to be the same as mFileName.

Can I get your r=? Thanks.

Comment 38

[av (gone)]

Why not to add mFullPath = nsnull; in the default constructor too?
r=av

Comment 39

[Marc Attinasi]

sr=attinasi

Comment 40

[selmer (gone)]

Please land this puppy on the *branch* right away!  Thanks for the fix!!

Comment 41

[av (gone)]

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

Comment 42

[Peter Lubczynski]

Fix is on the BRANCH!!!

I'll try to get it into the trunk as soon as I can.

Comment 43

[Peter Lubczynski]

Patch on both the trunk and the branch now, marking FIXED.

Comment 44

[Tracy Walker [:tracy]]

did the fix for the branch make it in on time to show on the 2001-07-18-03-0.9.2 
build?  I am still seeing this on that build.

Comment 45

[Peter Lubczynski]

Tracy,

I just downloaded Mac Classic 2001071803 mozilla.org bits and the above URL (and
others) work fo me. I will try the commercial ones off sweetlou now.

Comment 46

[Peter Lubczynski]

sorry, that WAS the 0.9.2 I downloaded from mozilla.org, not the trunk (but 8am
trunk should have it).

Comment 47

[Brian Nesse (gone)]

Tracy, WFM on Mac 2001-07-18-03-0.9.2 at www.disney.com. Where are you seeing
the problem occur?

Comment 48

[Tracy Walker [:tracy]]

the smoketest points me to http://www.tombraider.com/flash.html... there I click 
on flash version

Comment 49

[Brian Nesse (gone)]

I see no problems there either... something wierd here.

Comment 50

[Peter Lubczynski]

I see no problems with http://www.tombraider.com/flash.html
 on both the commercial and mozilla branch 3 am builds. 

Tracy, what URL did you download your build from?

Comment 51

[shrirang khanzode]

tried today's mac brnch build and this works now..tried a priliminary test and 
this test url...*calling Tracy now*

Comment 52

[Tracy Walker [:tracy]]

i retrieved the bits for the smoketests from sweetlou.

also, shrirang just dropped by my cube to confirm the bug.....he's going to 
check a few more mac machines...in the meantime I am rebooting to see if that 
helps and then I will get a build without the installer to see if that makes any 
differnece.

Comment 53

[Chris Petersen]

I'm using the 2001-07-18-03-0.9.2. Problem has been resolved for me too. Tested
under Mac OS 9.1.

Comment 54

[shrirang khanzode]

i could reproduce what Tracy said...*BUT that was using 0717 branch* 
(yesterday's build). Using today's build with this fixed...this is definitely 
fixed. tried a mac OS 9.1 and OS 8.6 and 9.0.4. Tracy, pls try reinstalling in a 
different location or something, let me know.

Comment 55

[shrirang khanzode]

nope, 8.6 IS showing this problem on today's mac branch (0718). Petersen can u 
pls reconfirm? reopening, changing OS.

Comment 56

[Chris Petersen]

Yes, confirming problem is still occuring under 8.6 with the 7/18 branch build.
Dialog appears which states plug-in required.

Comment 57

[Brian Nesse (gone)]

Chris, do you mean "It works the first time and the second time the dialog
appears which states plug-in required" or you get a dialog the first time you
try to go to the page?

Comment 58

[Chris Petersen]

The first time I go to the page I get the dialog.

Comment 59

[Brian Nesse (gone)]

That would imply that the plugin is simply failing to load under 8.6 then. Are
we sure that the plugin supports 8.6?

Comment 60

[Chris Petersen]

BTW, The flash plug-in does show up in the about:plug-ins window.

Comment 61

[Tracy Walker [:tracy]]

The machine I am seeing this on is a Mac 9.0.4 

Comment 62

[shrirang khanzode]

brian: yeah, flash used to work with Mac OS 8.6 (last tried on this OS was..last 
month or so)

Comment 63

[Brian Nesse (gone)]

Yeah, it probably will. If the plugin is in the plugins directory the directory
scan will pull in the information that is used by about:plugins. This does not,
however, attempt to instantiate an instance of the plugin. I'm guessing that the
failure to instantiate the plugin is somehow causing the default plugin to be
triggered. Of course, I'm only guessing...

Comment 64

[Brian Nesse (gone)]

Ok, if it used to work, then maybe we've somehow broken plug'ins on 8.6? Can you
try some other plugins and see what happens?

Comment 65

[Brian Nesse (gone)]

Just saw Tracy's response (why do our mail servers send stuff out of sync?). So,
this ONLY works on 9.1?

Comment 66

[shrirang khanzode]

i have a 9.0 and this works there too

Comment 67

[Tracy Walker [:tracy]]

I've been struggling with getting other plugins to download.  I used 4.x on 
shrirangs andvice and was able to get Quicktime loaded.  It works with the same 
build that was failing the Flash test.  But a new development has arised.  The 
flash player worked for one time at the tombraider site.  Returning there and it 
asks to "click here after downloading the plugin." Then going to Disney.com 
gives me the the downloader pluging download message. Then upon returning to the 
tombraider site the viewing area is blank...no alerts, no puzzle piece.

Comment 68

[Brian Nesse (gone)]

Allright! Now *that's* the bug that should have been fixed by this check in.

Comment 69

[selmer (gone)]

Do we have a patch that's ready for the branch?  The next build may be the last...

Comment 70

[Peter Lubczynski]

I believe this bug is now different than it's orriginal one, and here's the scoup:

This doesn't seem to fail only any OS 9.1 machine so yesterday I tried to set up
a debug envornment and build on an old G3 that I set up with OS 8.6. It should
be complete by the time I get into the office, ready to debug. Hopefully the
problem is not hard to fix.

Shrirang, have you been able to determine why this happens on Tracy's OS 9.0 and
not yours? I'm wondering if it's an certain extension or version of an extension
that's no updated. Perhaps it's CarbonLib.

Comment 71

[Peter Lubczynski]

Attachment: patch to fix plugin search v.1

Comment 72

[Peter Lubczynski]

The problem is that on OS 8.6 and some OS 9's, there is no Internet Plugins 
Folder in the System Folder or it's not supported and this was an error condition 
in the code. In the patch attached, I've moved the check to below the scan of the 
app's local plugins folder, and bail if we didn't find anything THEN.

I'm seeking reviews for this patch.

Comment 73

[Brian Nesse (gone)]

Won't that cause you to attempt to iterate on a bad "pluginsDir"? I think all
you want to do is skip the first loop if "pluginsDir" isn't valid.

Comment 74

[Peter Lubczynski]

Attachment: patch with Brian's comments and cleaned up tabbing

Comment 75

[Brian Nesse (gone)]

That's more like what I had in mind :).
r=bnesse.

Comment 76

[Marc Attinasi]

sr=attinasi

Comment 77

[Peter Lubczynski]

...waiting for clearence to checkin to branch.... (tinderbox still says closed
for verification)

Comment 78

[selmer (gone)]

Peter, pls check into the branch now!  Thanks.

Comment 79

[Peter Lubczynski]

Checked into both the trunk and the branch, marking FIXED.

Comment 80

[Tracy Walker [:tracy]]

verified fixed on mac commercial builds 2001-07-20-03-0.9.2 & 
2001-07-20-08-trunk

Comment 81

[hirata masakazu]

I am wondering if this latest fix is causing bug 91617.