Last Comment Bug 313347 - Flash plugin crashes browser on Intel Mac OS X
: Flash plugin crashes browser on Intel Mac OS X
Status: RESOLVED FIXED
[camino-1.0]
: fixed1.8.0.1, fixed1.8.0.2, fixed1.8.1
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: Future
Assigned To: Mark Mentovai
:
Mentors:
: 330747 (view as bug list)
Depends on:
Blocks: 324651
  Show dependency treegraph
 
Reported: 2005-10-21 21:26 PDT by Josh Aas
Modified: 2006-04-07 05:51 PDT (History)
18 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.1+
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Stack trace (29.97 KB, text/plain)
2005-10-24 20:05 PDT, Kaya Bekiroglu
no flags Details
Don't free TVector glue that wasn't allocated (976 bytes, patch)
2005-10-30 14:35 PST, Eric Albert
mark: review+
sfraser_bugs: superreview+
dveditz: approval1.8.0.1+
Details | Diff | Review
Stack Trace (24.64 KB, text/plain)
2006-01-13 02:06 PST, Myzar
no flags Details
Flash plugin workaround (7.74 KB, patch)
2006-01-14 15:46 PST, Mark Mentovai
no flags Details | Diff | Review
Flash plugin workaround, cleaned up (9.39 KB, patch)
2006-01-16 16:26 PST, Mark Mentovai
jaas: review+
bryner: superreview+
Details | Diff | Review
As checked in (9.43 KB, patch)
2006-02-15 13:12 PST, Mark Mentovai
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Review

Description Josh Aas 2005-10-21 21:26:28 PDT
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 2005-10-24 20:05:36 PDT
Created attachment 200701 [details]
Stack trace
Comment 2 Eric Albert 2005-10-26 20:32:55 PDT
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 2005-10-27 11:21:50 PDT
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 2005-10-27 11:42:21 PDT
I filed bug 314070 to get rid of the TVector glue in NSPR.
Comment 5 Eric Albert 2005-10-30 11:10:46 PST
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 2005-10-30 12:17:22 PST
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 2005-10-30 14:35:52 PST
Created attachment 201379 [details] [diff] [review]
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 2005-10-30 17:25:46 PST
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 2005-10-30 17:46:50 PST
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 2005-10-30 17:52:46 PST
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 2005-10-30 18:13:05 PST
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 2005-10-30 18:18:19 PST
(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 2005-10-30 22:16:07 PST
(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 2005-10-30 22:37:34 PST
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 2005-10-30 22:45:57 PST
(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 2005-11-04 18:26:14 PST
landed "Don't free TVector glue that wasn't allocated" on the trunk.
Comment 17 Michelle Sintov 2005-11-09 18:07:51 PST
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 2005-11-11 07:59:29 PST
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 2005-11-22 09:20:09 PST
Moving my stability bugs to blocking1.8.0.1?
Comment 20 Mark Mentovai 2005-12-28 10:12:28 PST
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] 2006-01-05 12:02:48 PST
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 2006-01-06 10:07:11 PST
Landed "Don't free TVector glue that wasn't allocated" on MOZILLA_1_8_BRANCH and MOZILLA_1_8_0_BRANCH.
Comment 23 Myzar 2006-01-13 02:06:26 PST
Created attachment 208361 [details]
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 2006-01-13 02:17:55 PST
(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 2006-01-13 11:24:24 PST
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 2006-01-14 15:40:06 PST
Reopening for workaround, which isn't planned to go into cvs.
Comment 27 Mark Mentovai 2006-01-14 15:46:15 PST
Created attachment 208517 [details] [diff] [review]
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 2006-01-14 16:21:28 PST
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 2006-01-16 16:26:13 PST
Created attachment 208680 [details] [diff] [review]
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 2006-01-17 06:26:09 PST
"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 2006-01-20 10:10:35 PST
Comment on attachment 208680 [details] [diff] [review]
Flash plugin workaround, cleaned up

r+, but not for actually landing
Comment 32 Mark Mentovai 2006-01-20 10:38:42 PST
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 2006-01-20 10:39:19 PST
Unofficial test builds with this patch are available at http://wiki.mozilla.org/Mac:Intel .
Comment 34 Mark Mentovai 2006-02-02 19:00:42 PST
Landed on CAMINO_1_0_BRANCH.
Comment 35 Mark Mentovai 2006-02-15 10:21:54 PST
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 2006-02-15 13:12:01 PST
Created attachment 212025 [details] [diff] [review]
As checked in

Contains an updated comment.
Comment 37 Mark Mentovai 2006-02-15 13:12:35 PST
Fixed on the trunk.
Comment 38 Mark Mentovai 2006-02-15 16:26:27 PST
Comment on attachment 212025 [details] [diff] [review]
As checked in

Checked in on MOZILLA_1_8_BRANCH.
Comment 39 Daniel Veditz [:dveditz] 2006-02-21 15:23:12 PST
Comment on attachment 212025 [details] [diff] [review]
As checked in

approved for 180 branch, a=dveditz for drivers
Comment 40 Mark Mentovai 2006-02-21 19:35:57 PST
Flash plugin workaround is now on 1_8_0 too.
Comment 41 Phil Ringnalda (:philor) 2006-03-16 20:56:46 PST
*** Bug 330747 has been marked as a duplicate of this bug. ***
Comment 42 Pu7o 2006-04-07 01:25:26 PDT
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 2006-04-07 05:51:20 PDT
Bug 333121 for comment 42.

Note You need to log in before you can comment on or make changes to this bug.