Remove support for async plugin init

ASSIGNED
Assigned to

Status

()

Core
Plug-ins
ASSIGNED
3 months ago
2 days ago

People

(Reporter: bsmedberg, Assigned: njn)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(23 attachments)

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
njn
: review?
jimm
Details | Diff | Splinter Review
5.26 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
2.27 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
4.98 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
62.38 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
3.00 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
1.98 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
1.74 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
1.73 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
7.18 KB, patch
njn
: review?
aklotz
Details | Diff | Splinter Review
(Reporter)

Description

3 months 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

3 months 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 months ago
Created attachment 8856370 [details] [diff] [review]
(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.
Attachment #8856370 - Flags: review?(aklotz)
(Assignee)

Updated

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

Comment 5

2 months ago
Created attachment 8856371 [details] [diff] [review]
(part 2) - Remove nsPluginTag::mSupportsAsyncInit

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

Comment 6

2 months 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 months 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

a month 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 9

a month ago
Created attachment 8869315 [details] [diff] [review]
(part 3) - Remove PluginModuleChromeParent::mAsyncInitError

It's unused.
Attachment #8869315 - Flags: review?(aklotz)
(Assignee)

Comment 10

a month ago
Created attachment 8869316 [details] [diff] [review]
(part 4) - Remove dom.ipc.plugins.asyncInit.enabled pref

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

Comment 11

a month ago
Created attachment 8869317 [details] [diff] [review]
(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.
Attachment #8869317 - Flags: review?(aklotz)
(Assignee)

Comment 12

a month ago
Created attachment 8869318 [details] [diff] [review]
(part 6) - Remove AsyncNPP_New

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

Comment 13

a month ago
Created attachment 8869319 [details] [diff] [review]
(part 7) - Remove AsyncNPP_NewStream
Attachment #8869319 - Flags: review?(aklotz)
(Assignee)

Comment 14

a month ago
Created attachment 8869320 [details] [diff] [review]
(part 8) - Remove AsyncNPP_NewResult
Attachment #8869320 - Flags: review?(aklotz)
(Assignee)

Comment 15

a month ago
Created attachment 8869321 [details] [diff] [review]
(part 9) - Remove AsyncNP_Initialize
Attachment #8869321 - Flags: review?(aklotz)
(Assignee)

Comment 16

a month ago
Created attachment 8870301 [details] [diff] [review]
(part 10) - Remove PluginModuleChromeParent::mContentParent
Attachment #8870301 - Flags: review?(aklotz)
(Assignee)

Comment 17

a month ago
Created attachment 8870302 [details] [diff] [review]
(part 11) - Remove LoadPluginResult and AssociatePluginId
Attachment #8870302 - Flags: review?(aklotz)
(Assignee)

Comment 18

a month ago
Created attachment 8870344 [details] [diff] [review]
(part 12) - Remove NP_InitializeResult
Attachment #8870344 - Flags: review?(aklotz)
(Assignee)

Comment 19

29 days 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

29 days ago
Can the first parts land independently?

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

Comment 21

28 days 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 22

28 days ago
Created attachment 8871607 [details] [diff] [review]
(part 13) - Remove surrogate use when casting
Attachment #8871607 - Flags: review?(aklotz)
(Assignee)

Comment 23

28 days ago
Created attachment 8871608 [details] [diff] [review]
(part 14) - Remove more surrogate use when casting
Attachment #8871608 - Flags: review?(aklotz)
(Assignee)

Comment 24

28 days ago
Created attachment 8871611 [details] [diff] [review]
(part 15) - Remove PluginModuleChromeParent::mInitOnAsyncConnect
Attachment #8871611 - Flags: review?(aklotz)
(Assignee)

Comment 25

28 days ago
Created attachment 8871612 [details] [diff] [review]
(part 16) - Remove PluginModuleChromeParent::mAsyncInitRv
Attachment #8871612 - Flags: review?(aklotz)
(Assignee)

Comment 26

28 days 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 27

25 days ago
Created attachment 8872202 [details] [diff] [review]
(part 17) - Remove AsyncNPP_NewStreamResult
Attachment #8872202 - Flags: review?(aklotz)
(Assignee)

Comment 28

25 days ago
Created attachment 8872203 [details] [diff] [review]
(part 18) - Remove PluginAsyncSurrogate

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

Comment 29

25 days ago
Created attachment 8872261 [details] [diff] [review]
(part 19) - Remove PluginModuleParent::mNPInitialized
Attachment #8872261 - Flags: review?(aklotz)
(Assignee)

Comment 30

25 days ago
Created attachment 8872262 [details] [diff] [review]
(part 20) - Remove nsPluginInstanceOwner::NotifyHostAsyncInitFailed
Attachment #8872262 - Flags: review?(aklotz)
(Assignee)

Comment 31

25 days ago
Created attachment 8872263 [details] [diff] [review]
(part 21) - Remove PluginModuleParent::mIsNPShutdownPending
Attachment #8872263 - Flags: review?(aklotz)
(Assignee)

Comment 32

25 days ago
Created attachment 8872264 [details] [diff] [review]
(part 22) - PluginModuleParent::mAsyncNewRv
Attachment #8872264 - Flags: review?(aklotz)
(Assignee)

Comment 33

25 days ago
Created attachment 8872265 [details] [diff] [review]
(part 23) - Remove PluginProcessParent::mRunCompleteTaskImmediately

Because it never gets set true any more.

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

Comment 34

25 days 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

25 days ago
aklotz, these patches are in a state ready for reviewing.
(Assignee)

Comment 36

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

Comment 37

18 days ago
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

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

Updated

17 days ago
Attachment #8856370 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

17 days ago
Attachment #8856371 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

17 days ago
Attachment #8869315 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

17 days ago
Attachment #8869316 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

17 days ago
Attachment #8869317 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

17 days ago
Attachment #8869318 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Updated

17 days ago
Attachment #8869319 - Flags: review?(aklotz) → review?(jmathies)

Updated

14 days ago
Attachment #8856370 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8856371 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8869315 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8869316 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8869317 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8869318 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8869319 - Flags: review?(jmathies) → review+

Updated

10 days ago
Attachment #8869320 - Flags: review?(aklotz) → review?(jmathies)

Updated

7 days ago
Attachment #8869320 - Flags: review?(jmathies) → review+

Updated

7 days ago
Attachment #8869321 - Flags: review?(aklotz) → review+

Updated

7 days ago
Attachment #8870301 - Flags: review?(aklotz) → review+

Updated

7 days ago
Attachment #8870302 - Flags: review?(aklotz) → review?(jmathies)

Updated

3 days ago
Attachment #8870302 - Flags: review?(jmathies) → review+

Updated

3 days 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

3 days ago
Attachment #8871608 - Flags: review?(aklotz) → review?(jmathies)
(Assignee)

Comment 41

2 days 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 days 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.)
You need to log in before you can comment on or make changes to this bug.