Closed Bug 1352573 Opened 7 years ago Closed 7 years ago

Remove NPN_PluginThreadAsyncCall

Categories

(Core Graveyard :: Plug-ins, enhancement, P3)

enhancement

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: benjamin, Assigned: n.nethercote)

References

Details

Attachments

(2 files, 5 obsolete files)

NPN_PluginThreadAsyncCall was an NPAPI extension specifically for Java to avoid threading issues. No other plugin has used it and since we only support Flash now we can remove it!

This is really nice because there's so much bookkeeping associated with this feature that we can remove.

This feature is implemented in two places.

The in-process codepath:
nsNPAPIPlugin.cpp _pluginthreadsynccall and the supporting code sPluginThreadAsyncCallLock and sPendingAsyncCalls


The out-of-process codepath:
PluginModuleChild.cpp _pluinthreadasynccall
PluginInstanceChild::AsyncCall and mAsyncCallMutex
ChildAsyncCall.h/cpp

*however*: it appears that the landing of flash async init (which is preffed off) caused our own internal code to use the async call codepath. So I'm going to mark this bug as blocked by another bug to back out most or all of async plugin init.
Depends on: 1352575
This patch removes a lot of stuff, but not everything that comment 1 says could
be removed. Please look for the "njn:" comments in the patch for things I'm
unsure about, and for where we might be able to do better.
Attachment #8885107 - Flags: feedback?(benjamin)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8885107 [details] [diff] [review]
Remove NPN_PluginThreadAsyncCall() and related machinery

># HG changeset patch
># User Nicholas Nethercote <nnethercote@mozilla.com>
># Date 1499753495 -36000
>#      Tue Jul 11 16:11:35 2017 +1000
># Node ID 92470c384a4a9bcd48145ddc291bba2b1cad60de
># Parent  317331a50bde2f2e59bcd6074078c1d6ec2bd20c
>Bug 1352573 - Remove NPN_PluginThreadAsyncCall() and related machinery. r=bsmedberg.
>
>diff --git a/dom/plugins/base/npapi.h b/dom/plugins/base/npapi.h

>+// njn: I changed NPNetscapeFuncs. Is that ok so long as I bumped this too?
>+#define NP_VERSION_MINOR 30

No. Flash is compiled against the current API and we should consider this frozen. It's fine to have this be a null pointer in the existing struct, but we shouldn't modify npapi.h/npfunctions.h (they are imported from upstream npapi-sdk anyway).

>+// njn: Can't remove this because it's used by AsyncCallbackAutoLock(), which
>+// is used by nsNPAPIPluginInstance::Stop(). It's possible that this usage is
>+// bogus -- it's protecting mRunning and mStopTime, neither of which are
>+// protected by a Mutex when accessed in other methods. Furthermore,
>+// sPluginThreadAsyncCallLock certainly looks like it was intended to protect
>+// sPendingAsyncCalls (which this patch removes) and nothing else.

I'm pretty certain that this mutex only exists to protect the asynccalls list. Please try to rip out this lock also.

>diff --git a/dom/plugins/ipc/ChildAsyncCall.h b/dom/plugins/ipc/ChildAsyncCall.h

>+// njn: Can't remove this yet because FlashThrottleAsyncMsg still inherits from
>+// it

ugh. That's not triggered from another thread, though, is it?

I guess we can move this to a followup, though.

>diff --git a/dom/plugins/test/mochitest/test_GCrace.html b/dom/plugins/test/mochitest/test_GCrace.html

>+      // njn: this test is now failing because checkGCRace no longer exists.
>+      // Just remove this entire test?

no, but see below

>diff --git a/dom/plugins/test/mochitest/test_npn_asynccall.html b/dom/plugins/test/mochitest/test_npn_asynccall.html

>   function runTests() {
>     var plugin = document.getElementById("plugin1");
>+    // njn: this test is now failing because asyncCallbackTest no longer exists.
>+    // Just remove this entire test?

Yes.

>diff --git a/dom/plugins/test/testplugin/nptest.cpp b/dom/plugins/test/testplugin/nptest.cpp

>-bool
>-checkGCRace(NPObject* npobj, const NPVariant* args, uint32_t argCount,
>-	    NPVariant* result)
>-{
>-  if (1 != argCount || !NPVARIANT_IS_OBJECT(args[0]))
>-    return false;
>-
>-  NPP npp = static_cast<TestNPObject*>(npobj)->npp;
>-
>-  NPObject* localFunc =
>-    NPN_CreateObject(npp, const_cast<NPClass*>(&kGCRaceClass));
>-
>-  GCRaceData* rd =
>-    new GCRaceData(npp, NPVARIANT_TO_OBJECT(args[0]), localFunc);
>-  NPN_PluginThreadAsyncCall(npp, FinishGCRace, rd);
>-
>-  OBJECT_TO_NPVARIANT(localFunc, *result);
>-  return true;
>-}

This test is still testing something important, and the async call here is an implementation detail and isn't central to the test. See https://bugzilla.mozilla.org/show_bug.cgi?id=542263#c9 for some old details about what this is testing. I open to arguments that we're unlikely to break this again before plugins are completely dead; but I also think we could keep this test with an alternate event instead of an async-call to trigger the race.
Attachment #8885107 - Flags: feedback?(benjamin)
Round 2.

- I reverted the npapi.h and npfunctions.h changes.

- I removed sPluginThreadAsyncCallLock and some associated functions.
  nsNPAPIPluginInstance::Stop()'s modifications of mRunning and mStopTime are
  now not protected by a lock.
  
- I removed test_npn_asynccall.html.

- I reinstated the GCRace test, but checkGCRace() needs something to replace
  the NPN_PluginThreadAsyncCall() call. I don't know what that something should
  be.

Fewer "njn:" comments than before, but there are still some in there.
Attachment #8885554 - Flags: feedback?(benjamin)
Attachment #8885107 - Attachment is obsolete: true
> >+// njn: Can't remove this yet because FlashThrottleAsyncMsg still inherits from
> >+// it
> 
> ugh. That's not triggered from another thread, though, is it?
> 
> I guess we can move this to a followup, though.

What would the follow-up look like? If the FlashThrottle stuff is removable, it would be nicer to do it as a precursor.
Flags: needinfo?(benjamin)
Flash throttling is important to keep, but there's no particular reason it needs to use this Asyc callback class structure. It can be a simple cancelable runnable on a single thread, I believe.
Flags: needinfo?(benjamin)
I don't know much about this stuff, but this looks plausible.
Attachment #8886568 - Flags: review?(benjamin)
ChildAsyncCall is now gone. The only remaining issue is what to do in
checkGCRace().
Attachment #8886570 - Flags: review?(benjamin)
Attachment #8885554 - Attachment is obsolete: true
Attachment #8885554 - Flags: feedback?(benjamin)
Attachment #8886570 - Attachment is obsolete: true
Attachment #8886570 - Flags: review?(benjamin)
Comment on attachment 8886568 [details] [diff] [review]
(part 1) - Convert FlashThrottleAsyncMsg from a ChildAsyncCall to a CancelableRunnable

>-    // We reuse ChildAsyncCall so we get the cancelation work
>-    // that's done in Destroy.

Removal of this comment makes me think we need to keep that functionality. We need to cancel this runnable on instance shutdown, which means holding a reference to it. So probably in the header around PluginInstanceChild::mPendingAsyncCalls (which is presumably going away in the other patch which I haven't read yet) we should have a RefPtr<FlashThrottleMsg> for the current pending message (there should only ever be one), and then cancel it in PluginInstanceChild::Destroy in place of this code: https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/dom/plugins/ipc/PluginInstanceChild.cpp#4306

That is (I believe) the only way for the raw PluginInstanceChild* mInstance; to be safe.
Attachment #8886568 - Flags: review?(benjamin) → review-
Comment on attachment 8886572 [details] [diff] [review]
(part 2) - Remove NPN_PluginThreadAsyncCall() and related machinery

r=me except for the question about checkGCRace().
Attachment #8886572 - Flags: review?(benjamin) → review+
For gcRace: I think we could schedule the callback using platform-specific code: SetTimer(NULL, 0, timerfunc) on Windows, NSTimer on cocoa. I'd be ok with this test only running on some platforms since it's really about testing a GC case which shouldn't be platform-specific.
New version adds a RefPtr<FlashThrottleMsg> as requested. It also adds a
FlashThrottleMsg::Cancel() which nulls mInstance, which is important.

Still a couple of things I'm uncertain about, which are described in "njn:"
comments.
Attachment #8887899 - Flags: review?(benjamin)
Attachment #8886568 - Attachment is obsolete: true
Comment on attachment 8887899 [details] [diff] [review]
(part 1) - Convert FlashThrottleAsyncMsg from a ChildAsyncCall to a CancelableRunnable

>+    // njn: Not sure about this. Can we only have one such message at a time,
>+    // or do subsequent messages displace not-yet-run messages?
>+    MOZ_ASSERT(!mFlashThrottleMsg);

I'm actually not sure. I think I've only ever seen one at a time, but I wouldn't want to count on Flash behavior here. Maybe there's one per instance.

So I guess that we'd either need to keep a linked list or an array of references... gah.

Good catch, I wasn't thinking about this carefully.
Attachment #8887899 - Flags: review?(benjamin)
Another attempt! I've added mPendingFlashThrottleMsgs, which is much like
mPendingAsyncCalls, but just for flash throttle messages.
Attachment #8888760 - Flags: review?(benjamin)
Attachment #8887899 - Attachment is obsolete: true
Attachment #8888760 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4b5196987ab37beb9a81b6c46ef2ecbec27609
Bug 1352573 (part 1) - Convert FlashThrottleAsyncMsg from a ChildAsyncCall to a CancelableRunnable. r=bsmedberg.
Keywords: leave-open
Whiteboard: BLOCKED by undoing parts of async init
Attachment #8888760 - Flags: checkin+
Mentor: benjamin
https://hg.mozilla.org/integration/mozilla-inbound/rev/db05998a8a032109be3afe6066518cdf903c13e0
Bug 1352573 (part 2) - Remove NPN_PluginThreadAsyncCall() and related machinery. r=bsmedberg.
Keywords: leave-open
Attachment #8886572 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/db05998a8a03
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Hi.

Just tried to build Firefox from current state of mozilla-central sources and found that npn.pluginthreadasynccall is now NULL. I have a plugin wrapper that allows to run PPAPI version of Flash. And it uses npn.pluginthreadasynccall a lot. (Some quirks of PPAPI requires wrapper to have own "main" thread to be able to reenter message loop.)

Is there a chance to restore npn.pluginthreadasynccall implementation? Or is there a way to safely call browser functions from a plugin from any thread?

Of course, there is official NPAPI version from Adobe, that works without npn.pluginthreadasynccall. But it lacks 3d and video decoding acceleration. And it's unlikely it ever will implement that.
Flags: needinfo?(n.nethercote)
Rinat: Flash support in Firefox is gradually being phased out, as described here: https://blog.mozilla.org/futurereleases/2017/07/25/firefox-roadmap-flash-end-life/

Although the functionality removal done by this bug is not explicitly listed in the roadmap document, the general trend is that Flash functionality is being reduced, and this bug fits within that general trend. Apologies for the inconvenience this causes you.

> Or is there a way to safely call browser functions from a plugin from any thread?

I don't know. Jim, do you know?
Flags: needinfo?(n.nethercote) → needinfo?(jmathies)
> Flash functionality is being reduced

I thought it was about Flash usage being reduced (which is fine), not its functionality (which is not fine). But that's not the point.

Honestly, I was surprised when it turned out that Flash is not using NPN_PluginThreadAsyncCall. That just seems to be a bug. There is a warning in API description [1] that says browser part of API should be called exclusively from main thread. The only function that is allowed to be called from other threads is NPN_PluginThreadAsyncCall.

Lets assume we have a video to show. On every frame plugin needs browser to issue a paint event. There is no way to do it from main thread itself, as it's owned by a browser (or plugin-container process) and plugin needs to wait there. So, separate thread is required. How Flash solves this without NPN_PluginThreadAsyncCall? Calling directly? That would lead to race conditions.

I think removing NPN_PluginThreadAsyncCall while keeping requirement that every other call should be performed from the main thread breaks NPAPI in a bad way.

[1] https://developer.mozilla.org/en-US/docs/Plugins/Guide/Browser_Side_Plug-in_API
> How Flash solves this without NPN_PluginThreadAsyncCall? Calling directly? That would lead to race conditions.

OK, I think I found how libflashplayer.so solves this. It doesn't call browser functions from other threads. All calls are in main thread only. Solution is quite simple. And it's both smart and dumb at the same time.

With help of g_timeout_add() called from the main thread, libflashplayer.so registers a callback that is called from GLib main loop with one millisecond interval. Up to thousand times a second GLib main loop wakes up to call a function which apparently calls other functions, scheduled to be called on the main thread.

But why? It would be a clever trick which allowed multi-threaded operation with API not designed to be thread-aware. But in presence of NPN_PluginThreadAsyncCall it looks like solving a non-existent bug.

Anyway, that makes things a bit easier, as it's possible to emulate NPN_PluginThreadAsyncCall in plugin itself. But I still would like to have NPN_PluginThreadAsyncCall back. Just to have a consistent API.
(In reply to Nicholas Nethercote [:njn] from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 6d4b5196987ab37beb9a81b6c46ef2ecbec27609
> Bug 1352573 (part 1) - Convert FlashThrottleAsyncMsg from a ChildAsyncCall
> to a CancelableRunnable. r=bsmedberg.

FYI all of this throttling code can go away. Adobe confirmed years ago they fixed the internal bug this code addressed.

(In reply to Nicholas Nethercote [:njn] from comment #20)
> Rinat: Flash support in Firefox is gradually being phased out, as described
> here:
> https://blog.mozilla.org/futurereleases/2017/07/25/firefox-roadmap-flash-end-
> life/
> 
> Although the functionality removal done by this bug is not explicitly listed
> in the roadmap document, the general trend is that Flash functionality is
> being reduced, and this bug fits within that general trend. Apologies for
> the inconvenience this causes you.
> 
> > Or is there a way to safely call browser functions from a plugin from any thread?
> 
> I don't know. Jim, do you know?

Well, plugins run out of process by default, as sych they only have the main thread of the plugin process to work with, and AFAIA there are no additional apis that support async like calls.
Flags: needinfo?(jmathies)
> (In reply to Nicholas Nethercote [:njn] from comment #20)
> 
> Well, plugins run out of process by default, as sych they only have the main
> thread of the plugin process to work with, and AFAIA there are no additional
> apis that support async like calls.

Still, blocking in something like NPP_HandleEvent causes all browser tabs to start showing a spinner icon. Even closing the tab with Flash player doesn't help. It's required to kill plugin-container process. (I was impatient and did not try to wait till timeouts kick in). So I guess something in browser still can block if plugin-container blocks.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.