1352575 - Remove support for async plugin init

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Benjamin Smedberg]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

Also:

* PluginInstanceParent::mUseSurrogate can go.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

* PluginModuleParent.cpp: PluginModuleMapping can go; it was only for e10s + async init

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove unused supportsAsyncInit arguments and fields

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsPluginTag::mSupportsAsyncInit

It's not used in any useful way.

Comment 6

[Nicholas Nethercote [inactive]]

FYI: I'm most of the way through this. I will likely finish it in the first half of next week.

Comment 7

[Benjamin Smedberg]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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...

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Remove PluginModuleChromeParent::mAsyncInitError

It's unused.

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove dom.ipc.plugins.asyncInit.enabled pref

The feature is in the process of being removed.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Remove PluginModuleParent::mIsStartingAsync

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.

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Remove AsyncNPP_New

And related code.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove AsyncNPP_NewStream

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Remove AsyncNPP_NewResult

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Remove AsyncNP_Initialize

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Remove PluginModuleChromeParent::mContentParent

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Remove LoadPluginResult and AssociatePluginId

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: (part 12) - Remove NP_InitializeResult

Comment 19

[Nicholas Nethercote [inactive]]

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?

Comment 20

[Benjamin Smedberg]

Can the first parts land independently?

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

Comment 21

[Nicholas Nethercote [inactive]]

> 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.

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: (part 13) - Remove surrogate use when casting

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: (part 14) - Remove more surrogate use when casting

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Remove PluginModuleChromeParent::mInitOnAsyncConnect

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: (part 16) - Remove PluginModuleChromeParent::mAsyncInitRv

Comment 26

[Nicholas Nethercote [inactive]]

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.

Comment 27

[Nicholas Nethercote [inactive]]

Attachment: (part 17) - Remove AsyncNPP_NewStreamResult

Comment 28

[Nicholas Nethercote [inactive]]

Attachment: (part 18) - Remove PluginAsyncSurrogate

The patch also removes PluginDataResolver and various other things that are no
longer necessary.

Comment 29

[Nicholas Nethercote [inactive]]

Attachment: (part 19) - Remove PluginModuleParent::mNPInitialized

Comment 30

[Nicholas Nethercote [inactive]]

Attachment: (part 20) - Remove nsPluginInstanceOwner::NotifyHostAsyncInitFailed

Comment 31

[Nicholas Nethercote [inactive]]

Attachment: (part 21) - Remove PluginModuleParent::mIsNPShutdownPending

Comment 32

[Nicholas Nethercote [inactive]]

Attachment: (part 22) - PluginModuleParent::mAsyncNewRv

Comment 33

[Nicholas Nethercote [inactive]]

Attachment: (part 23) - Remove PluginProcessParent::mRunCompleteTaskImmediately

Because it never gets set true any more.

The patch also removes PluginModuleChromeParent::WaitForIPCConnection().

Comment 34

[Nicholas Nethercote [inactive]]

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.

Comment 35

[Nicholas Nethercote [inactive]]

aklotz, these patches are in a state ready for reviewing.

Comment 36

[Nicholas Nethercote [inactive]]

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf87238d370242191a2b3e841a684de56f02494c

Comment 37

[Jim Mathies [:jimm]]

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.

Comment 38

[Nicholas Nethercote [inactive]]

Thanks, Jim. I'll r? on the first few to get the ball rolling.

Comment 39

[Jim Mathies [:jimm]]

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'.)

Comment 40

[Jim Mathies [:jimm]]

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?

Comment 41

[Nicholas Nethercote [inactive]]

> 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.

Comment 42

[Nicholas Nethercote [inactive]]

> 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 43

[Jim Mathies [:jimm]]

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.

Comment 44

[Nicholas Nethercote [inactive]]

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

Yes, in part 18.

Comment 45

[Jim Mathies [:jimm]]

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

Comment 46

[Nicholas Nethercote [inactive]]

Thank you for all the reviews. I will land these patches early next week.

Comment 47

[Nicholas Nethercote [inactive]]

Oh, I see I need to rework part 15; I will do that first :)

Comment 48

[Nicholas Nethercote [inactive]]

Attachment: (part 15) - Remove PluginModuleChromeParent::mInitOnAsyncConnect

Part 15 with the unnecessary checks removed.

Comment 49

[Nicholas Nethercote [inactive]]

Attachment: Roll-up of all patches

jimm, requested a roll-up of all the patches. aklotz, you might want to take a look at this too?

Comment 50

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c6fc72a333f24d397e96fdafc936d60763706d

Comment 51

[Nicholas Nethercote [inactive]]

I plan to land these patches tomorrow, unless I hear suggestions that I shouldn't.

Comment 52

[Nicholas Nethercote [inactive]]

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.

Comment 53

[Nicholas Nethercote [inactive]]

Landed version:

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

I filed bug 1378644 for removing the things I didn't remove in this bug.

Comment 54

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3024611da04a
https://hg.mozilla.org/mozilla-central/rev/102d714a9d84
https://hg.mozilla.org/mozilla-central/rev/ae71b34e0b43
https://hg.mozilla.org/mozilla-central/rev/f6bd1a90ea8f
https://hg.mozilla.org/mozilla-central/rev/527d5254f927
https://hg.mozilla.org/mozilla-central/rev/690c244100f1
https://hg.mozilla.org/mozilla-central/rev/ab8e6999302e
https://hg.mozilla.org/mozilla-central/rev/ed0500b57625
https://hg.mozilla.org/mozilla-central/rev/b63ec391bedf
https://hg.mozilla.org/mozilla-central/rev/4628f97e8178
https://hg.mozilla.org/mozilla-central/rev/61f16a79c955
https://hg.mozilla.org/mozilla-central/rev/e609e1af5673
https://hg.mozilla.org/mozilla-central/rev/df2eb29996da
https://hg.mozilla.org/mozilla-central/rev/f698238a6459
https://hg.mozilla.org/mozilla-central/rev/4bde5a3cdde8
https://hg.mozilla.org/mozilla-central/rev/35a1e5e900b4
https://hg.mozilla.org/mozilla-central/rev/81134f853f15
https://hg.mozilla.org/mozilla-central/rev/6b0f6053943f
https://hg.mozilla.org/mozilla-central/rev/2ea30798f7ba
https://hg.mozilla.org/mozilla-central/rev/ae7695ceea32
https://hg.mozilla.org/mozilla-central/rev/1eec63116867
https://hg.mozilla.org/mozilla-central/rev/7a0ce01666ec
https://hg.mozilla.org/mozilla-central/rev/737ecf36bccd