Remove support for async plugin init

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: benjamin, Assigned: njn)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(24 attachments, 1 obsolete attachment)

8.53 KB, patch
jimm
: review+
Details | Diff | Splinter Review
8.41 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.37 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.61 KB, patch
jimm
: review+
Details | Diff | Splinter Review
35.04 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.23 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.57 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.66 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.05 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.36 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.78 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.50 KB, patch
jimm
: review+
Details | Diff | Splinter Review
15.97 KB, patch
jimm
: review+
Details | Diff | Splinter Review
6.32 KB, patch
jimm
: review+
Details | Diff | Splinter Review
2.27 KB, patch
jimm
: review+
Details | Diff | Splinter Review
4.98 KB, patch
jimm
: review+
Details | Diff | Splinter Review
62.38 KB, patch
jimm
: review+
Details | Diff | Splinter Review
3.00 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.98 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.74 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.73 KB, patch
jimm
: review+
Details | Diff | Splinter Review
7.18 KB, patch
jimm
: review+
Details | Diff | Splinter Review
5.33 KB, patch
jimm
: review+
Details | Diff | Splinter Review
160.39 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Async plugin init was a feature we tried to enable where the Flash plugin was not synchronously initialized with respect to the page that loaded it. It was bug 998863.

This feature was risky and we never got it into a shippable state. Now that we're moving Flash to click-to-play by default, that risk is unnecessary.

The feature is currently landed in-tree but off by default. In order to simplify a bunch of other code, I'd like us to try and back it out completely. This will e.g. unblock bug 1352573 but also make some things more readable.

aklotz, I'm not asking you to do this, but could you provide advice and maybe mentoring about whether it's possible to actually "back out" individual things from bug 998863 or whether we should read those patches as inspiration for hand-removing code, or some other process.
Flags: needinfo?(aklotz)
(Reporter)

Updated

2 years ago
Depends on: 998863
While those patches could definitely provide a roadmap, it's probably best to illustrate the fundamentals here:

* dom/plugins/ipc/PluginAsyncSurrogate* can go. Anything that references PluginAsyncSurrogate or AsyncNPObject can go. Note that there are some references to the affected classes in dom/plugins/base too.

* dom/plugins/ipc/PluginDataResolver.h can optionally go, but you'll need to clean up anything that casts to a PluginDataResolver and replace that with casts to PluginInstanceParent. Look for PluginInstanceParent::Cast and PluginModuleParent::StreamCast.

* nsPluginTag::mSupportsAsyncInit can go

* The declaration of kAsyncInitPref in PluginModuleParent can go, as well as its declaration in all.js

* In PluginModuleParent code, mAsyncInitRv mAsyncInitError mInitOnAsyncConnect mIsStartingAsync mContentParent can all go. Any code that is conditional on those values existing and being true can go.

* AssociatePluginId and LoadPluginResult may be removed from dom/ipc/PContent.ipdl

* AsyncNP_Initialize, AsyncNPP_New, NP_InitializeResult may be removed from PPluginModule.ipdl

* AsyncNPP_NewStreamResult may be removed from PBrowserStream.ipdl

* AsyncNPP_NewResult and AsyncNPP_NewStream may be removed from PPluginInstance.ipdl

I'd tend to think that once that stuff and anything that depends on it is gone, you should be left with sync-only plugin init.
Flags: needinfo?(aklotz)
Also:

* PluginInstanceParent::mUseSurrogate can go.
* PluginModuleParent.cpp: PluginModuleMapping can go; it was only for e10s + async init
(Assignee)

Comment 4

2 years ago
The aAllowAsyncInit argument to PluginModuleParent's constructor is unused.
This patch removes it and various other things that are no longer needed once
it is gone.
Attachment #8856370 - Flags: review?(aklotz)
(Assignee)

Updated

2 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
It's not used in any useful way.
Attachment #8856371 - Flags: review?(aklotz)
(Assignee)

Comment 6

2 years ago
FYI: I'm most of the way through this. I will likely finish it in the first half of next week.
(Reporter)

Comment 7

2 years ago
njn (when you get back), how's this going? I have a volunteer contributor who might like to help either with this or with bug1 352573 which might be blocked on parts of this bug.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 8

2 years ago
I've done all of the points Aaron mentioned in comment 1 and comment 2; I haven't yet done the one from comment 3. I also have done very little testing, and I need to do a big final check.

I could upload my patches and hand them over if that's helpful...
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 10

2 years ago
The feature is in the process of being removed.
Attachment #8869316 - Flags: review?(aklotz)
(Assignee)

Comment 11

2 years ago
This allows a bunch of other things to be removed too, including
PluginModuleParent::mSurrogateInstances,
PluginModuleChromeParent::sInstantiated, and NS_PLUGIN_INIT_PENDING.

The patch also removes the AsyncPluginInit crash annotation.
Attachment #8869317 - Flags: review?(aklotz)
(Assignee)

Comment 12

2 years ago
And related code.
Attachment #8869318 - Flags: review?(aklotz)
(Assignee)

Comment 13

2 years ago
Attachment #8869319 - Flags: review?(aklotz)
(Assignee)

Comment 14

2 years ago
Attachment #8869320 - Flags: review?(aklotz)
(Assignee)

Comment 15

2 years ago
Attachment #8869321 - Flags: review?(aklotz)
(Assignee)

Comment 19

2 years ago
aklotz, the 12 patches I've posted take us about halfway, and they look good on try. I then have another 11 or so patches but I'm having some trouble with test failures that I don't understand on part 13, so that's currently blocking progress.

How do you think we should proceed? Do the patches I've posted so far look like they're going in the right direction? Should I upload part 13 for you to look at?
Flags: needinfo?(aklotz)
(Reporter)

Comment 20

2 years ago
Can the first parts land independently?

I'm happy to look over test failures if that would help.
(Assignee)

Comment 21

2 years ago
> Can the first parts land independently?

I don't see why not.
 
> I'm happy to look over test failures if that would help.

Thanks! I will post the remaining patches and the links to the failing try push.
(Assignee)

Comment 26

2 years ago
I was able to fix the test failures I was seeing. The first 16 parts now look good on try. I still need to test the remaining 8 or so parts.
(Assignee)

Comment 28

2 years ago
The patch also removes PluginDataResolver and various other things that are no
longer necessary.
Attachment #8872203 - Flags: review?(aklotz)
(Assignee)

Comment 33

2 years ago
Because it never gets set true any more.

The patch also removes PluginModuleChromeParent::WaitForIPCConnection().
Attachment #8872265 - Flags: review?(aklotz)
(Assignee)

Comment 34

2 years ago
That's where I'm going to end my patches. There are two missing things that I'm not planning on doing because I've spent more than enough time on this already:

- Remove LaunchedTask. I had a patch to do this, but it has test failures.

- Remove PluginModuleMapping.
Flags: needinfo?(aklotz)
(Assignee)

Comment 35

2 years ago
aklotz, these patches are in a state ready for reviewing.
Hey Nick, if you want to chat about this ping me. Aaron is busy on e10s+a11y and won't be able to get to these reviews for a while. If you want you can farm these out to other dom peers who would be willing. I reviewed a lot of this so I'd be happy to help.
(Assignee)

Comment 38

2 years ago
Thanks, Jim. I'll r? on the first few to get the ball rolling.
(Assignee)

Updated

2 years ago
Attachment #8856370 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8856371 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8869315 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8869316 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8869317 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8869318 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8869319 - Flags: review?(aklotz) → review?(jmathies)

Updated

2 years ago
Attachment #8856370 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8856371 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869315 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869316 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869317 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869318 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869319 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869320 - Flags: review?(aklotz) → review?(jmathies)

Updated

2 years ago
Attachment #8869320 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8869321 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8870301 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8870302 - Flags: review?(aklotz) → review?(jmathies)

Updated

2 years ago
Attachment #8870302 - Flags: review?(jmathies) → review+

Updated

2 years ago
Attachment #8870344 - Flags: review?(aklotz) → review+
Comment on attachment 8871607 [details] [diff] [review]
(part 13) - Remove surrogate use when casting

Review of attachment 8871607 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1657,5 @@
>      //  (3) remove both parent and child IDs from map
>      //  (4) free parent
>  
>      PLUGIN_LOG_DEBUG_FUNCTION;
> +    PluginInstanceParent* i = PluginInstanceParent::Cast(instance);

nit - lets standardize the variable name we use here, I see 'parentInstance', 'i', 'pi', etc.. Cleaning that up would be great. (How about 'pip'? Not of fan of 'i'.)
Attachment #8871607 - Flags: review?(aklotz) → review+
Comment on attachment 8871608 [details] [diff] [review]
(part 14) - Remove more surrogate use when casting

Review of attachment 8871608 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1688,5 @@
>    NPP npp = nullptr;
>    if (NS_FAILED(mInstance->GetNPP(&npp)) || !npp) {
>      return;
>    }
> +  mozilla::plugins::PluginAsyncSurrogate::NotifyDestroyPending(npp);

these changes here seem a bit odd, why are we introducing PluginAsyncSurrogate use here?

Updated

2 years ago
Attachment #8871608 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Comment 41

2 years ago
> nit - lets standardize the variable name we use here, I see
> 'parentInstance', 'i', 'pi', etc.. Cleaning that up would be great. 

I did that! I made things more consistent.

> (How about 'pip'? Not of fan of 'i'.)

Ok. I agree that 'i' isn't a good name. It was the most common one, but while in here I might as well fix things properly.
(Assignee)

Comment 42

2 years ago
> these changes here seem a bit odd, why are we introducing
> PluginAsyncSurrogate use here?

We aren't. This change just adds some namespace qualification.

This is necessary because elsewhere the patch removes a 'using mozilla::plugins::PluginAsyncSurrogate' statement. (Note that there was some unified builds bootlegging happening, which explains why this happened across .cpp file boundaries.)
Comment on attachment 8871608 [details] [diff] [review]
(part 14) - Remove more surrogate use when casting

Review of attachment 8871608 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1688,5 @@
>    NPP npp = nullptr;
>    if (NS_FAILED(mInstance->GetNPP(&npp)) || !npp) {
>      return;
>    }
> +  mozilla::plugins::PluginAsyncSurrogate::NotifyDestroyPending(npp);

I'm going to assume I'll see this removed in a patch farther down the list.
Attachment #8871608 - Flags: review?(jmathies) → review+
(Assignee)

Comment 44

2 years ago
> I'm going to assume I'll see this removed in a patch farther down the list.

Yes, in part 18.

Updated

2 years ago
Attachment #8871611 - Flags: review?(aklotz) → review?(jmathies)
Comment on attachment 8871611 [details] [diff] [review]
(part 15) - Remove PluginModuleChromeParent::mInitOnAsyncConnect

Review of attachment 8871611 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +2030,5 @@
>      mNPPIface = pFuncs;
>  
>      if (!mSubprocess->IsConnected()) {
> +        // The subprocess isn't connected yet.
> +        // njn: what to do here?

We don't need this check, afaict, since the process is created when the plugin module is constructed. looking at the original landing for reference - 

https://hg.mozilla.org/mozilla-central/rev/8a4c2bf5ec09#l6.914

https://bugzilla.mozilla.org/show_bug.cgi?id=998863#c94
Attachment #8871611 - Flags: review?(jmathies) → review-

Updated

2 years ago
Attachment #8871612 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872202 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872203 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872261 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872262 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872263 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872264 - Flags: review?(aklotz) → review+

Updated

2 years ago
Attachment #8872265 - Flags: review?(aklotz) → review+

Updated

2 years ago
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 46

2 years ago
Thank you for all the reviews. I will land these patches early next week.
Flags: needinfo?(n.nethercote)
(Assignee)

Comment 47

2 years ago
Oh, I see I need to rework part 15; I will do that first :)
(Assignee)

Comment 48

2 years ago
Part 15 with the unnecessary checks removed.
Attachment #8882938 - Flags: review?(jmathies)
(Assignee)

Updated

2 years ago
Attachment #8871611 - Attachment is obsolete: true
(Assignee)

Comment 49

2 years ago
jimm, requested a roll-up of all the patches. aklotz, you might want to take a look at this too?
(Assignee)

Updated

2 years ago
Attachment #8882941 - Flags: feedback?(jmathies)
Attachment #8882941 - Flags: feedback?(aklotz)

Updated

2 years ago
Attachment #8882938 - Flags: review?(jmathies) → review+
(Assignee)

Comment 51

2 years ago
I plan to land these patches tomorrow, unless I hear suggestions that I shouldn't.
Flags: needinfo?(jmathies)
Flags: needinfo?(aklotz)
(Assignee)

Comment 52

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3024611da04acca25dda97079dd877b884cf13fe
Bug 1352575 (part 1) - Remove unused supportsAsyncInit arguments and fields. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/102d714a9d8498ee509707c4996df9afe931b2b8
Bug 1352575 (part 2) - Remove nsPluginTag::mSupportsAsyncInit. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae71b34e0b43de7c359774883787ad192d85cba5
Bug 1352575 (part 3) - Remove PluginModuleChromeParent::mAsyncInitError. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6bd1a90ea8f57a45d3b80ae23d42fbd63aecb3f
Bug 1352575 (part 4) - Remove dom.ipc.plugins.asyncInit.enabled pref. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/527d5254f92739ad0d5d3859ea338ac534d4a728
Bug 1352575 (part 5) - Remove PluginModuleParent::mIsStartingAsync. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/690c244100f13c5f55191db614b4be668d458318
Bug 1352575 (part 6) - Remove AsyncNPP_New. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8e6999302e50a62b93db1d938334108b0e8dbc
Bug 1352575 (part 7) - Remove AsyncNPP_NewStream. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed0500b5762558d797594794a7d46af4a6a75b58
Bug 1352575 (part 8) - Remove AsyncNPP_NewResult. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b63ec391bedf06d615492ce0621b874987b9fd20
Bug 1352575 (part 9) - Remove AsyncNP_Initialize. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4628f97e817819d3f9deb778a32157f3c54abecb
Bug 1352575 (part 10) - Remove PluginModuleChromeParent::mContentParent. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/61f16a79c955a087757a42d0d8cb137538150f41
Bug 1352575 (part 11) - Remove LoadPluginResult and AssociatePluginId. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e609e1af56731ae68923da4380a4d2764ee381c0
Bug 1352575 (part 12) - Remove NP_InitializeResult. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/df2eb29996da332947433b083da2b2793de5c6a5
Bug 1352575 (part 13) - Remove surrogate use when casting. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f698238a64591edbce3e44a234ce87a348e47731
Bug 1352575 (part 14) - Remove more surrogate use when casting. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bde5a3cdde862c0c01855378bbd8a10ea82e273
Bug 1352575 (part 15) - Remove PluginModuleChromeParent::mInitOnAsyncConnect. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/35a1e5e900b431fa0df2c99b3a819b743cbf18e3
Bug 1352575 (part 16) - Remove PluginModuleChromeParent::mAsyncInitRv. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/81134f853f15b1e4a0c009aefc23e9ce740f8c4b
Bug 1352575 (part 17) - Remove AsyncNPP_NewStreamResult. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0f6053943f47823620aa32da53f6e5b5263ef3
Bug 1352575 (part 18) - Remove PluginAsyncSurrogate. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea30798f7ba85aa5073af8dca2d560abe201d70
Bug 1352575 (part 19) - Remove PluginModuleParent::mNPInitialized. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae7695ceea329fb25d37f03175c51416815af61e
Bug 1352575 (part 20) - Remove nsPluginInstanceOwner::NotifyHostAsyncInitFailed. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eec63116867162fddeef9e3cae204ee734a86fb
Bug 1352575 (part 21) - Remove PluginModuleParent::mIsNPShutdownPending. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0ce01666ec2415beb99d63681422e4518aed7a
Bug 1352575 (part 22) - PluginModuleParent::mAsyncNewRv. r=jimm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/737ecf36bccdb653c604d8ca361b468fa5faf5cc
Bug 1352575 (part 23) - Remove PluginProcessParent::mRunCompleteTaskImmediately. r=jimm.
(Assignee)

Updated

2 years ago
Blocks: 1378644
(Assignee)

Comment 53

2 years ago
Landed version:

> 37 files changed, 130 insertions(+), 2274 deletions(-)

I filed bug 1378644 for removing the things I didn't remove in this bug.
No longer blocks: 1378644
Flags: needinfo?(jmathies)
Flags: needinfo?(aklotz)
(Assignee)

Updated

2 years ago
Attachment #8882941 - Flags: feedback?(jmathies)
Attachment #8882941 - Flags: feedback?(aklotz)
You need to log in before you can comment on or make changes to this bug.