Closed Bug 629401 Opened 13 years ago Closed 13 years ago

Carbon plugins that cannot load in X64 Mac should send an event or notifyObservers of failure

Categories

(Core Graveyard :: Plug-ins, defect)

x86_64
macOS
defect
Not set
normal

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: ddahl, Assigned: jaas)

References

Details

(Whiteboard: [hardblocker])

Attachments

(1 file, 2 obsolete files)

In bug 628651, (which is the front-end of this issue) we need to notify users when their 64bit Mac OSX tries to load a 32bit carbon plugin.

At this point our code just gives up loading a mismatched plugin:

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleChild.cpp#1880

It would be great if we can utilize the same method for dealing with crashed plugins by triggering an event in the document that attempted to load the plugin, as in these cases. see: https://bugzilla.mozilla.org/show_bug.cgi?id=628651#c7

A more simple approach may be to notify Observers of this kind of failure as in:

https://bugzilla.mozilla.org/show_bug.cgi?id=628651#c10
Blocks: 628651
Requesting blocking as this blocks a hardblocker: bug 628651
blocking2.0: --- → ?
Josh, can you look into this? This is a hardblocker since it blocks a hardblocker.
blocking2.0: ? → betaN+
Whiteboard: [hardblocker]
Assignee: nobody → joshmoz
I haven't tested this beyond compiling but it should send an event when an OOPP instance fails because it tried to negotiate the Carbon event model.

In order for this to be useful we probably also want a boolean attribute on the plugin host called "i386SupportsCarbonNPAPI". We'd only put up an alert about restarting if this is true. We can then use the architecture scanning code in nsPluginsDirDarwin to find out whether or not we have an i386 child binary. If we do have an i386 child binary then we can just assume it supports Carbon NPAPI.

David - can you confirm that this notification works properly for you? Once we have it working we can move onto the "i386SupportsCarbonNPAPI" attribute in a separate patch.
Attachment #508186 - Flags: review?(ddahl)
(In reply to comment #3)
> Created attachment 508186 [details] [diff] [review]
> Carbon NPAPI failure notification v1.0
> 
> I haven't tested this beyond compiling but it should send an event when an OOPP
> instance fails because it tried to negotiate the Carbon event model.

When I tried to build it I got this failure:

/Users/moco/code/mozilla/dom/plugins/PluginModuleChild.cpp: In member function ‘virtual bool mozilla::plugins::PluginModuleChild::AnswerPPluginInstanceConstructor(mozilla::plugins::PPluginInstanceChild*, const nsCString&, const uint16_t&, const InfallibleTArray<nsCString>&, const InfallibleTArray<nsCString>&, NPError*)’:
/Users/moco/code/mozilla/dom/plugins/PluginModuleChild.cpp:1890: error: ‘class mozilla::plugins::PluginInstanceChild’ has no member named ‘CallNegotiatedCarbon’
PluginProcessParent.cpp

Is there a new method missing in this patch?
or - is CallNegotiatedCarbon some kind of getter for the NegotiatedCarbon ipdl method?
s/CallNegotiatedCarbon/SendNegotiatedCarbon/
Attachment #508186 - Attachment is obsolete: true
Attachment #508573 - Flags: review?(ddahl)
Attachment #508186 - Flags: review?(ddahl)
Whiteboard: [hardblocker] → [hardblocker][has patch]
Whiteboard: [hardblocker][has patch] → [hardblocker][has patch][b11]
Removing from b11 radar, but hoping for ETA of Friday.
Whiteboard: [hardblocker][has patch][b11] → [hardblocker][has patch][ETA Feb-04]
What plugins are there that we can test this with?
I thought the test plugin was a carbon plugin
(In reply to comment #9)
> I thought the test plugin was a carbon plugin

Looks to be cocoa in 64-bit and carbon (maybe both?) in 32 bit. In which case it should work just fine in 64-bit mode.

I tried the test plugin from the 1.9.2 branch but it seems Minefield decided that that was invalid for some reason.
Comment on attachment 508573 [details] [diff] [review]
Carbon NPAPI failure notification v1.1

>diff --git a/dom/plugins/PluginModuleChild.cpp b/dom/plugins/PluginModuleChild.cpp
>--- a/dom/plugins/PluginModuleChild.cpp
>+++ b/dom/plugins/PluginModuleChild.cpp
>@@ -1878,16 +1878,23 @@ PluginModuleChild::AnswerPPluginInstance
>     }
> 
> #if defined(XP_MACOSX) && defined(__i386__)
>     // If an i386 Mac OS X plugin has selected the Carbon event model then
>     // we have to fail. We do not support putting Carbon event model plugins
>     // out of process. Note that Carbon is the default model so out of process
>     // plugins need to actively negotiate something else in order to work
>     // out of process.
>+
>+    // Send notification that a plugin tried to negotiate Carbon NPAPI so that
>+    // users can be notified that restarting the browser in i386 mode may allow
>+    // them to use the plugin.
>+    childInstance->SendNegotiatedCarbon();
>+
>+    // Fail to instantiate.
>     if (childInstance->EventModel() == NPEventModelCarbon) {
>         *rv = NPERR_MODULE_LOAD_FAILED_ERROR;
>     }
> #endif
> 
>     return true;
> }
> 

Am I mistaken or should the SendNegotiatedCarbon call be inside the if block there? This does send the event that we want in frontend but it appears to do it even for some working plugins (Quicktime), I assume because they are 32-bit but cocoa.
Attachment #508573 - Flags: review?(ddahl) → review-
Whiteboard: [hardblocker][has patch][ETA Feb-04] → [hardblocker][needs new patch][ETA Feb-04]
Yeah, oops.
Attachment #508573 - Attachment is obsolete: true
Attachment #509119 - Flags: review?(dtownsend)
Comment on attachment 509119 [details] [diff] [review]
Carbon NPAPI failure notification v1.2

This works well for me. Presumably we need a plugin peer to review this before it can land?
Attachment #509119 - Flags: review?(dtownsend) → review+
Comment on attachment 509119 [details] [diff] [review]
Carbon NPAPI failure notification v1.2

If Benoit can review that is good enough.
Attachment #509119 - Flags: review?(b56girard)
Comment on attachment 509119 [details] [diff] [review]
Carbon NPAPI failure notification v1.2

Looks good!
Attachment #509119 - Flags: review?(b56girard) → review+
mossop, ddahl: Can you guys land my patch here whenever you land the code that uses the event?
Whiteboard: [hardblocker][needs new patch][ETA Feb-04] → [hardblocker][has patch][ETA Feb-04]
Landed: http://hg.mozilla.org/mozilla-central/rev/69f3807d4bb4
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker][has patch][ETA Feb-04] → [hardblocker]
Target Milestone: --- → mozilla2.0b12
This broke non-libxul builds (bug 631412).  :(
Comment on attachment 509119 [details] [diff] [review]
Carbon NPAPI failure notification v1.2

>+  nsString type = NS_LITERAL_STRING("npapi-carbon-event-model-failure");
Unfortunately this copies the string. Perhaps you wanted
NS_NAMED_LITERAL_STRING(type, "npapi-carbon-event-model-failure");
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: