313347 - Flash plugin crashes browser on Intel Mac OS X

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Josh Aas]

At least as of Intel Mac OS X build 8F1099, Apple provides a universal binary
Flash plugin. When it is used (macromedia.com), Firefox crashes. Firefox does
not crash using Apple's universal binary Quicktime plugin. Not sure if this is
our bug or Apple's. I'll attach a stack trace when I get a chance.

Comment 1

[Kaya Bekiroglu]

Attachment: Stack trace

Comment 2

[Eric Albert]

When I looked at this briefly this past weekend, it looked like Firefox was using some TVector code to talk to plugins.  That won't work for Intel...which makes me wonder how the QuickTime plugin works.

I probably won't have a chance to get back to this till this coming weekend, though.

Comment 3

[Mark Mentovai]

The TVector stuff should only come into play on PowerPC.

http://lxr.mozilla.org/mozilla1.8/source/modules/plugin/base/src/ns4xPlugin.cpp#198

198 #if defined(XP_MACOSX) && defined(__POWERPC__)
...
238 #else
240 #define TV2FP(f) (f)
241 #define FP2TV(f) (f)

I do see some TVector glue in NSPR, but I don't think it's getting in our way here.

Comment 4

[Mark Mentovai]

I filed bug 314070 to get rid of the TVector glue in NSPR.

Comment 5

[Eric Albert]

I was confused by the presence of gNetscapeFuncsGlueTable in the backtrace, which only exists in the TVector code.  I'm not sure why that symbol is showing up at all in object files for Intel builds, but that's not worth worrying about right now.  Anyway, the address which shows up as gNetscapeFuncsGlueTable is actually inside the Flash plugin (which of course doesn't have symbols).

Firefox is crashing because fCallbacks, rather than being a pointer to funky TVector glue or a list of function pointers, is instead a list of pointers to function pointers.  Rather than dereferencing the pointers to get the function pointers, we're trying to execute the code at the site of the pointers and hilarity ensues.

Hardcoding a fix for this would be trivial, but it's more interesting to figure out why the code doesn't work.  One oddity here is that Firefox is initializing Mac plugins differently from how Safari does it.  It's possible that there's a bug in the Flash plugin here -- after all, this is the first Mach-O Flash plugin and if the code path Firefox is using for initialization isn't tested by Safari I wouldn't be surprised if it was broken.  If there's a contact at Macromedia I could talk to who works on Flash for Mac OS X, we could probably get this resolved much faster.

Comment 6

[Mark Mentovai]

Sounds like fun.  Are other current osx86 plug-ins working?

I filed bug 314432 to remove the TVector paste from the Default Plugin.  I hadn't thought about it before because it's a plugin and because it's disabled in Firefox.  We should fix it because other products do use it and because we can do better than offering plugin developers a broken sample.

Comment 7

[Eric Albert]

Attachment: Don't free TVector glue that wasn't allocated

The QuickTime plugin works, but Flash doesn't.  A little bit of disassembly shows that the QuickTime plugin's "main" function returns a list of function pointers, but the Flash plugin's "main" function returns a list of pointers to function pointers.  WebKit doesn't call "main"; it calls "NP_GetEntryPoints", as the Windows and OS/2 cases do in ns4xPlugin::ns4xPlugin().  Switching to that case appears to get the Flash plugin to get a little bit farther, but I'm not sure how many of the other XP_MACOSX cases in ns4xPlugin.cpp then need to be changed.  And regardless, the Flash plugin should return the right data from any valid plugin API.

All of that aside, this patch removes the free() calls that deallocate the TVector glue in ns4xPlugin.cpp.  Since we have no TVector glue on Intel, unallocated objects are getting freed.  This shows up when using the QuickTime plugin as warning messages from malloc in the console.

Comment 8

[Mark Mentovai]

Comment on attachment 201379 [details] [diff] [review]
Don't free TVector glue that wasn't allocated

That's very interesting.  Looking at the WebKit source (WebKit/Plugins.subproj/WebBasePluginPackage.m), it selects between calling NP_GetEntryPoints and main depending on the plugin type.  An API change has quietly been made, and we might want to follow suit in ns4xPlugin.

Comment 9

[Eric Albert]

I'm not sure that's an API change so much as use of the modern plugin API for modern plugins.  WebKit still calls main for CFM plugins, and before the introduction of the universal Flash plugin, Flash was CFM-only.  The Firefox code doesn't seem to distinguish between CFM and Mach-O in the same way as the WebKit code does, which results in things like TVector glue being created for Mach-O plugins that don't need it.

Actually, there's a way to test all of this that doesn't require using an Intel-based Mac.  Acquire a copy of the universal Flash plugin, then try to use it with the PowerPC version of Firefox.  You should see the same failure as I'm seeing on my Intel system.  Then if you wanted to, say, add code to distinguish between Mach-O and CFM plugins and use the Windows code path for Mach-O plugins (which seems like it'd be correct), you could write it and fix it there and not worry much about whole Intel thing.  I'm not going to volunteer to do that, though -- I don't know enough about how Firefox works to do that.

Comment 10

[Eric Albert]

That brings up the point that if Macromedia releases this version of the Flash plugin for Mac OS X, it probably will crash Firefox on PowerPC in the same way as it crashes on Intel today.  So making this work is a bit more important, in that it'll affect Firefox on all Mac OS X systems, not just the ones that aren't shipping yet.  It also means that if the decision is made to switch to the modern plugin API for Mach-O plugins, that work has to be done for Mach-O plugins on PowerPC as well, not just for Intel.

Comment 11

[Mark Mentovai]

The thin Flash Player 8 on ppc is Mach-O but loads via a CFM shim.  I wouldn't be surprised if that's how the fat version loads too on ppc.  Considering how our plugin API pushes tvectors around in native Mach-O, it wouldn't be all that surprising.  That's why I'm finding the idea of using NP_GetEntryPoints and raw Mach-O pointers so enticing for both architectures.  (We'd have to overcome some NSPRisms to work out the library type...)

Don't worry, Eric, I'd gladly take this on - or maybe Josh would.

This almost-kinda-sorta is a tangent that deserves its own bug.

Comment 12

[Tinic Uro]

(In reply to comment #10)
> That brings up the point that if Macromedia releases this version of the Flash
> plugin for Mac OS X, it probably will crash Firefox on PowerPC in the same way
> as it crashes on Intel today. 

Just to clarify, we will not release it this way, especially if it causes any problems with current browsers on PowerPC (this includes FireFox 1.0 to 1.5). This Intel version is merely a proof of concept for Apple, not beta quality. This port to Intel & XCode (meaning it uses gcc instead of CodeWarrior) was not done or fully tested by Macromedia. On the other hand it'll help greatly if we can take up on the chance to switch to more modern APIs during the transition to Intel.

Comment 13

[Simon Fraser [no longer active]]

(In reply to comment #11)
> The thin Flash Player 8 on ppc is Mach-O but loads via a CFM shim.  I wouldn't
> be surprised if that's how the fat version loads too on ppc.  Considering how
> our plugin API pushes tvectors around in native Mach-O, it wouldn't be all that
> surprising.  That's why I'm finding the idea of using NP_GetEntryPoints and raw
> Mach-O pointers so enticing for both architectures.  (We'd have to overcome
> some NSPRisms to work out the library type...)

I would have a little cautious when doing this, in case there are plugins that go "oh, main was called, this must be Mozilla/Netscape". They might use that to change their drawing or event handling strategy to fit different browsers.

Comment 14

[Tinic Uro]

Flash Player 8 (which is Mach-O) uses the PLUGIN_TO_HOST_GLUE macro. Which is TVector glue stuff. Very much as recommended here:

http://www.mozilla.org/projects/plugins/plugin_scripting_ABI_technote.html

e.g. our code looks like this:

...
pluginFuncs->newp = NewNPP_NewProc(PLUGIN_TO_HOST_GLUE(newp, Private_New));
pluginFuncs->destroy = NewNPP_DestroyProc(PLUGIN_TO_HOST_GLUE(destroy, Private_Destroy));
...

There is no specific checks for Mozilla/FireFox or Safari in our main() I can make out.

Comment 15

[Eric Albert]

(In reply to comment #14)
> Flash Player 8 (which is Mach-O) uses the PLUGIN_TO_HOST_GLUE macro. Which is
> TVector glue stuff. Very much as recommended here:
> 
> http://www.mozilla.org/projects/plugins/plugin_scripting_ABI_technote.html

Ah, well, that'd explain the SetupTVtoFPGlue calls I see in the disassembly.  Oy, vey.  That page is very badly wrong for Intel.  The simplest fix is probably to change this:

    #if TARGET_RT_MAC_MACHO

to this:

    #if TARGET_RT_MAC_MACHO && TARGET_CPU_PPC

If you wouldn't mind doing that and sending me a test build, I'll let you know if it works.  Thanks!

Comment 16

[Josh Aas]

landed "Don't free TVector glue that wasn't allocated" on the trunk.

Comment 17

[Michelle Sintov]

Indeed, removing the TVector code and reverting to the pre-Mach-O-pointers-to-TVector-and-vice-versa world makes MacIntel Deer Park play Flash movies. I will send this proof-of-concept plug-in to Mozilla (Josh and Simon), Apple (Eric) and Opera (who got dinged by the same issue).

Comment 18

[Mark Mentovai]

Nominating to get attachment 201379 [details] [diff] [review] on the 1.8 branch followup.  Other than that, this bug is probably INVALID for our purposes.

Comment 19

[Mark Mentovai]

Moving my stability bugs to blocking1.8.0.1?

Comment 20

[Mark Mentovai]

Comment on attachment 201379 [details] [diff] [review]
Don't free TVector glue that wasn't allocated

This has been on the trunk since 1104.  Nominating for 1.8.0.1.  This is needed on x86 Macs, it prevents freeing memory that was never allocated.

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 201379 [details] [diff] [review]
Don't free TVector glue that wasn't allocated

affects only intel-mac

a=dveditz for 1.8/1.8.0.1 branches. Please add the fixed1.8.1 and fixed1.8.0.1 keywords when checked in.

Comment 22

[Josh Aas]

Landed "Don't free TVector glue that wasn't allocated" on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.

Comment 23

[Myzar]

Attachment: Stack Trace

Is this supposed to be fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8) Gecko/20060112 Firefox/1.5

i still crash with the attached stack on www.macromedia.com with this build

Comment 24

[Eric Albert]

(In reply to comment #23)
> Is this supposed to be fixed in Mozilla/5.0 (Macintosh; U; Intel Mac OS X;
> en-US; rv:1.8) Gecko/20060112 Firefox/1.5
> 
> i still crash with the attached stack on www.macromedia.com with this build

That's expected.  This will crash until Adobe releases a Flash plug-in for Mac OS X which doesn't use TVector glue on Intel.

The patch that went in for this bug wasn't directly related to Flash crashing, despite the title of the bug.

Comment 25

[Josh Aas]

Flash still crashes the browser in the official 10.4.4 release for Intel, and there is nothing we can really do about it. We have to wait until they ship an updated flash plugin, which should be in < 60 days. That is around the time we ship official Firefox 1.5.0.2, so people will just have to have the updated plugin. Until then, unofficial Intel builds from me will have Flash disabled (they will not allow the plugin to load).

We plan to rewrite our NSAPI code to not ever call main() on a mach-o plugin, but that is a big job that we can't start for another couple months.

Comment 26

[Mark Mentovai]

Reopening for workaround, which isn't planned to go into cvs.

Comment 27

[Mark Mentovai]

Attachment: Flash plugin workaround

This is a workaround for the shipping (broken) version of the Flash plugin.  There are two problems with the way the plugin behaves with Mozilla:

http://lxr.mozilla.org/mozilla1.8/source/modules/plugin/base/src/ns4xPlugin.cpp#439

When the plugin's main function is called, it returns pointers to function pointers in np_callbacks.

The plugin internally builds and calls through a table of CFM TVectors.  The table is built using ns4xPlugin::CALLBACKS as source.

This workaround patch detects the broken Flash plugin by name and bundle version, and confirms its detection by looking for TVector glue where there shouldn't be any.  If the plugin is identified as broken, the function pointer pointers are dereferenced to function pointers, and the CFM TVectors are translated into corresponding jumps in x86 machine code.

This patch is intended to be used in x86 Mozilla betas, and isn't intended at this time to be included in any shipping final release or in cvs.  We hope that an updated version of the Flash plugin will be included in an Apple system update in the near future, obviating the need for this workaround.

Comment 28

[Mark Mentovai]

Comment on attachment 208517 [details] [diff] [review]
Flash plugin workaround

+      for (int i = 0 ; i < 40 ; i++) {

Should be changed for NSPR type conformance:

+      for (PRUint32 i = 0 ; i < 40 ; i++) {

and

+          *(PRUint32*) ((PRUint8*) base+1) =
+            ((PRUint8*) address - ((PRUint8*) base + 5));

Should be changed to reflect signedness of offset:

+          *(PRInt32*) ((PRUint8*) base+1) =
+            ((PRUint8*) address - ((PRUint8*) base + 5));

Comment 29

[Mark Mentovai]

Attachment: Flash plugin workaround, cleaned up

I cleaned this patch up to make the PPC-to-x86 jump table translation cleaner, and to add additional safety checks.

Comment 30

[Mark Mentovai]

"Flash plugin workaround" patches aren't currently intended to be committed to cvs.  That portion of the bug does not block 1.8.0.1, unless someone decides to make an official x86 Mac release from 1.8.0.1.

Comment 31

[Josh Aas]

Comment on attachment 208680 [details] [diff] [review]
Flash plugin workaround, cleaned up

r+, but not for actually landing

Comment 32

[Mark Mentovai]

Not landing in the tree - we'll patch unofficial builds locally for now and see where the pieces fall with respect to Apple shipping a fixed plugin by the time we're ready for an official x86 release.  Leaving open 'til then.

Comment 33

[Mark Mentovai]

Unofficial test builds with this patch are available at http://wiki.mozilla.org/Mac:Intel .

Comment 34

[Mark Mentovai]

Landed on CAMINO_1_0_BRANCH.

Comment 35

[Mark Mentovai]

Comment on attachment 208680 [details] [diff] [review]
Flash plugin workaround, cleaned up

Looks like we're actually going to need this.  Apple released 10.4.5 yesterday without updating the Flash plug-in, and there's been no release of a fixed Flash plug-in yet from Macromedia or Adobe.

This comment:

+// BROKEN_PLUGIN_HACK works around bugs in the version of the Macromedia
+// Flash Player plugin that ships with the initial consumer shipment of
+// Mac OS X 10.4.4 for x86-based Macs.

will change to reflect that 10.4.4 and 10.4.5 at the very least are affected.

Note that Camino 1.0 already shipped with this patch.

Comment 36

[Mark Mentovai]

Attachment: As checked in

Contains an updated comment.

Comment 37

[Mark Mentovai]

Fixed on the trunk.

Comment 38

[Mark Mentovai]

Comment on attachment 212025 [details] [diff] [review]
As checked in

Checked in on MOZILLA_1_8_BRANCH.

Comment 39

[Daniel Veditz [:dveditz]]

Comment on attachment 212025 [details] [diff] [review]
As checked in

approved for 180 branch, a=dveditz for drivers

Comment 40

[Mark Mentovai]

Flash plugin workaround is now on 1_8_0 too.

Comment 41

[Phil Ringnalda (:philor)]

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

Comment 42

[Pu7o]

A new flash plugin has been released with Mac OS X v10.4.6, and it seems to work fine without the workaround. Is this hack still needed?

Comment 43

[Mark Mentovai]

Bug 333121 for comment 42.