521118 - [OOPP] Need NPAPI threadsafety checks in plugin process

Status: RESOLVED FIXED

(Core :: IPC, defect)

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: dom/plugin/ checks with ipc/glue/ thrown in too for good measure

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Updated

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/projects/electrolysis/rev/91ae1b1cf67e is for debugging purposes only, it won't ship.  Leaving this bug open.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bent told me that the current nsNPAPI* code silently returns back to the plugin if the plugin calls an NPN_* method off the main thread.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Well, not "silent", but it doesn't abort or crash. See http://mxr.mozilla.org/mozilla-central/source/modules/plugin/base/src/nsNPAPIPlugin.cpp#731 for an example.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/message_loop.h#437 needs to be changed

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Found bug 554172, bug 554171, and bug 554178 along the way.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 434051 [details] [diff] [review]
Patch, v1

>+  return type == MessageLoop::TYPE_MOZILLA_UI;

That should clearly be TYPE_UI instead.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 434051 [details] [diff] [review]
Patch, v1

> void NP_CALLBACK
> _setexception(NPObject* aNPObj,
>               const NPUTF8* aMessage)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    ENSURE_PLUGIN_THREAD_VOID();
>     NS_NOTYETIMPLEMENTED("Implement me!");
> }
> 

Probably want to s/NS_NOTYETIMPLEMENTED/NS_WARNING/ while you're at it.

> void NP_CALLBACK
>@@ -1226,17 +1236,17 @@ _pluginthreadasynccall(NPP aNPP,
>                                                  aUserData));
> }
> 
> NPError NP_CALLBACK
> _getvalueforurl(NPP npp, NPNURLVariable variable, const char *url,
>                 char **value, uint32_t *len)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 

I don't understand the point of this, we're just going to crash on an RPC_ASSERT() if it gets into RPCChannel.

> NPError NP_CALLBACK
> _setvalueforurl(NPP npp, NPNURLVariable variable, const char *url,
>                 const char *value, uint32_t len)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 

Same here.

>@@ -1285,17 +1295,17 @@ _setvalueforurl(NPP npp, NPNURLVariable 
> NPError NP_CALLBACK
> _getauthenticationinfo(NPP npp, const char *protocol,
>                        const char *host, int32_t port,
>                        const char *scheme, const char *realm,
>                        char **username, uint32_t *ulen,
>                        char **password, uint32_t *plen)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 

And here.

> uint32_t NP_CALLBACK
> _scheduletimer(NPP npp, uint32_t interval, NPBool repeat,
>                void (*timerFunc)(NPP npp, uint32_t timerID))
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>-
>+    // Don't assert plugin thread here for consistency with in-process plugins.

Related issue: this is just going to crash in ipc/chromium code.

>     return InstCast(npp)->ScheduleTimer(interval, repeat, timerFunc);
> }
> 
> void NP_CALLBACK
> _unscheduletimer(NPP npp, uint32_t timerID)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
>     InstCast(npp)->UnscheduleTimer(timerID);

This can cause memory corruption.

> }
> 
> NPError NP_CALLBACK
> _popupcontextmenu(NPP instance, NPMenu* menu)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
>     NS_NOTYETIMPLEMENTED("Implement me!");

NS_WARNING again, maybe?

> NPBool NP_CALLBACK
> _convertpoint(NPP instance, 
>               double sourceX, double sourceY, NPCoordinateSpace sourceSpace,
>               double *destX, double *destY, NPCoordinateSpace destSpace)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
>     NS_NOTYETIMPLEMENTED("Implement me!");

Here too?

> void NP_CALLBACK
> PluginModuleChild::NPN_ReleaseObject(NPObject* aNPObj)
> {
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 
>     NPObjectData* d = current()->mObjectMap.GetEntry(aNPObj);
>     if (!d) {
>         NS_ERROR("Releasing object not in mObjectMap?");
>         return;
>     }
>

This isn't threadsafe, right?

> NPIdentifier NP_CALLBACK
> PluginModuleChild::NPN_GetStringIdentifier(const NPUTF8* aName)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 

Hmm, this seems to be on top of your NPIdentifiers patch.  If this method can do IPC, we'll die from an RPC_ASSERT() if this is on the wrong thread.

>     if (!aName)
>         return 0;
> 
>     PluginModuleChild* self = PluginModuleChild::current();
>     nsDependentCString name(aName);
> 
>     PluginIdentifierChild* ident;
>@@ -1634,17 +1643,17 @@ PluginModuleChild::NPN_GetStringIdentifi
> }
> 
> void NP_CALLBACK
> PluginModuleChild::NPN_GetStringIdentifiers(const NPUTF8** aNames,
>                                             int32_t aNameCount,
>                                             NPIdentifier* aIdentifiers)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 

Same here.

> NPIdentifier NP_CALLBACK
> PluginModuleChild::NPN_GetIntIdentifier(int32_t aIntId)
> {
>     PLUGIN_LOG_DEBUG_FUNCTION;
>-    AssertPluginThread();
>+    // Don't assert plugin thread here for consistency with in-process plugins.
> 

And here.

r=me.  I don't understand propagating known-buggy behavior, but it also doesn't seem like these checks matter much in practice for the whitelisted OOPP.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ok, fixed up.

http://hg.mozilla.org/mozilla-central/rev/b33e7b784570

Comment 12

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/811bb1e04a0e

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 14

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430