Closed
Bug 1352573
Opened 7 years ago
Closed 7 years ago
Remove NPN_PluginThreadAsyncCall
Categories
(Core Graveyard :: Plug-ins, enhancement, P3)
Core Graveyard
Plug-ins
Tracking
(firefox58 fixed)
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: benjamin, Assigned: n.nethercote)
References
Details
Attachments
(2 files, 5 obsolete files)
30.82 KB,
patch
|
benjamin
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
6.56 KB,
patch
|
benjamin
:
review+
n.nethercote
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8885107 -
Attachment is obsolete: true
Assignee | ||
Comment 4•7 years ago
|
||
> >+// 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)
Reporter | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
I don't know much about this stuff, but this looks plausible.
Attachment #8886568 -
Flags: review?(benjamin)
Assignee | ||
Comment 7•7 years ago
|
||
ChildAsyncCall is now gone. The only remaining issue is what to do in checkGCRace().
Attachment #8886570 -
Flags: review?(benjamin)
Assignee | ||
Updated•7 years ago
|
Attachment #8885554 -
Attachment is obsolete: true
Attachment #8885554 -
Flags: feedback?(benjamin)
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8886572 -
Flags: review?(benjamin)
Assignee | ||
Updated•7 years ago
|
Attachment #8886570 -
Attachment is obsolete: true
Attachment #8886570 -
Flags: review?(benjamin)
Reporter | ||
Comment 9•7 years ago
|
||
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-
Reporter | ||
Comment 10•7 years ago
|
||
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+
Reporter | ||
Comment 11•7 years ago
|
||
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.
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8886568 -
Attachment is obsolete: true
Reporter | ||
Comment 13•7 years ago
|
||
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)
Assignee | ||
Comment 14•7 years ago
|
||
Another attempt! I've added mPendingFlashThrottleMsgs, which is much like mPendingAsyncCalls, but just for flash throttle messages.
Attachment #8888760 -
Flags: review?(benjamin)
Assignee | ||
Updated•7 years ago
|
Attachment #8887899 -
Attachment is obsolete: true
Reporter | ||
Updated•7 years ago
|
Attachment #8888760 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d4b5196987ab37beb9a81b6c46ef2ecbec27609 Bug 1352573 (part 1) - Convert FlashThrottleAsyncMsg from a ChildAsyncCall to a CancelableRunnable. r=bsmedberg.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Updated•7 years ago
|
Whiteboard: BLOCKED by undoing parts of async init
Assignee | ||
Updated•7 years ago
|
Attachment #8888760 -
Flags: checkin+
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6d4b5196987a
Reporter | ||
Updated•7 years ago
|
Mentor: benjamin
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db05998a8a032109be3afe6066518cdf903c13e0 Bug 1352573 (part 2) - Remove NPN_PluginThreadAsyncCall() and related machinery. r=bsmedberg.
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Assignee | ||
Updated•7 years ago
|
Attachment #8886572 -
Flags: checkin+
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/db05998a8a03
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 19•6 years ago
|
||
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)
Assignee | ||
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
> 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
Comment 22•6 years ago
|
||
> 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.
Comment 23•6 years ago
|
||
(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)
Comment 24•6 years ago
|
||
> (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.
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•