998863 - Asynchronous Initialization of Out-of-process Plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

I've been doing a bunch of design and prototyping around this. I expect to be able to post some stuff for feedback once I return from PTO.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

At http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.cpp#814 there is code that is (reasonably modulo this bug) expecting the plugin to have been initialized at this point such that nsPluginInstanceOwner::CreateWidget may be called. In the async init case this assumption no longer holds and widget creation would need to be deferred until the plugin initialization has completed.

John, how badly would such a deferral affect things from the perspective of content?

Comment 2

[John Schoenick [:johns]]

(In reply to Aaron Klotz [:aklotz] from comment #1)
> At
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginHost.
> cpp#814 there is code that is (reasonably modulo this bug) expecting the
> plugin to have been initialized at this point such that
> nsPluginInstanceOwner::CreateWidget may be called. In the async init case
> this assumption no longer holds and widget creation would need to be
> deferred until the plugin initialization has completed.
> 
> John, how badly would such a deferral affect things from the perspective of
> content?

This code just creates the native widget for the plugin inside InstanceOwner, it doesn't call into the plugin that I can see. The subsequent SetWindow requires initialize to have completed, but that call just passes a started plugin its window, and can be moved. In fact, content might call setwindow immediately after instantiate anyway [1]. It's also called whenever the window parameters change IIRC, so it doesn't need any magic placement.

Note that the "instantiate" plugin paths are all very crufty, having had all the code above/under them shuffle around, so feel free to cleanup/refactor this stuff if needed, most of it is probably not laid out that way for a specific purpose.

[1] http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#847

Comment 3

[John Schoenick [:johns]]

(In reply to John Schoenick [:johns] from comment #2)
> It's also
> called whenever the window parameters change IIRC, so it doesn't need any
> magic placement.

Well, modulo the reference to bug 870216 three lines below that:
http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#851

Comment 4

[Florian Bender]

Would this encompass starting plugin-container only when it is needed? Currently I have the plugin process running although I just started Firefox, most plugins are disabled (except Silverlight on CTP) and there's no page loaded using any plugin.

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Florian Bender from comment #4)
> Would this encompass starting plugin-container only when it is needed?
> Currently I have the plugin process running although I just started Firefox,
> most plugins are disabled (except Silverlight on CTP) and there's no page
> loaded using any plugin.

If that's what you're seeing, I'd file a bug -- AFAIK plugin-container is always supposed to be launched on-demand. This bug is focusing on what happens once the decision to launch plugin-container has been set in motion.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Florian Bender from comment #4)
> Currently I have the plugin process running although I just started Firefox,
> most plugins are disabled (except Silverlight on CTP) and there's no page
> loaded using any plugin.

This sounds like the background thumbnail process, although i'm not sure if it's still called plugin-container. New bug is better for discussion anyway.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Very, very WIP

This patch is still a work in progress; there are lots of changes in the pipe but this is the most stable snapshot ATM. Works on Windows, definitely missing some stuff on Mac, untried on Linux :-)

Comment 10

[Harsh86]

What's the status of this feature?

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: nsJSNPRuntime patch

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: dom/plugins/base Patches

Comment 13

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: ipc/glue patches

Async Plugin Init needs a way to call into the channel and wait for a message to be received without nesting the main event loop. This patch is an attempt to provide such a capability.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: IPDL changes

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 5: PluginModuleChild changes

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 6: PluginInstanceChild changes

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 7: PluginDataResolver

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 8: Plugin process launcher changes

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 9: PluginModuleParent changes

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 10: PluginInstanceParent changes

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 11: IPC Browser stream changes

Comment 22

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 12: PluginScriptableObject changes

Comment 23

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 13: PluginAsyncSurrogate

Comment 24

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 14: Build system updates

Comment 25

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 15: Test updates

Comment 26

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

In this bug we need to be able to query whether the plugin element has a property without walking the protochain, so I'd like to add a NP_HasOwnProperty function to nsJSObjWrapper.

Comment 27

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521695 [details] [diff] [review]
Part 4: IPDL changes

This patch adds some async variants of some plugin calls. Of course, they need a way to return the result back to the parent, so there are async calls for that as well.

Comment 28

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521696 [details] [diff] [review]
Part 5: PluginModuleChild changes

These changes are needed to handle the revised IPDL in the previous patch.

Comment 29

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521698 [details] [diff] [review]
Part 6: PluginInstanceChild changes

Changes to PluginInstanceChild to handle the revised IPC.

Comment 30

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521700 [details] [diff] [review]
Part 7: PluginDataResolver

New class for resolving NPP::pdata. We cast to a PluginDataResolver and then call its methods to retrieve pointers to the actual object.

Comment 31

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521703 [details] [diff] [review]
Part 8: Plugin process launcher changes

These changes make launching of the plugin process asynchronous. A task is provided by the caller to be invoked when the launch has completed.

Comment 32

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521704 [details] [diff] [review]
Part 9: PluginModuleParent changes

This patch is one of the more complicated ones in this bug. It makes everything just work. Unfortunately some of the paths are complicated because it needs to support all permutations of (asyncinit,e10s) preferences.

Comment 33

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521706 [details] [diff] [review]
Part 10: PluginInstanceParent changes

These changes make PluginInstanceParent compatible with the new IPDL and implement a new Cast function that uses PluginDataResolover to return PluginInstanceParent and PluginAsyncSurrogate pointers.

Comment 34

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521708 [details] [diff] [review]
Part 12: PluginScriptableObject changes

Normally the async init code does not allow any scriptable object calls into the plugin until it has finished initializing. During testing I found that this breaks a lot of plugins who end up reentering themselves during a plugin to browser call.

I've written a class called PushSurrogateAcceptCalls that temporarily lifts the restrictions and allows a plugin to reenter itself, making the (fairly reasonable) assumption that a plugin that is willing to call into the browser in such a way is ready and able to accept reentry.

Comment 35

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521710 [details] [diff] [review]
Part 13: PluginAsyncSurrogate

This is the meat of the bug. The PluginModuleParent patch in part 9 redirects plugin calls to this object during the initialization process. As initialization reaches certain completion milestones, fewer calls are redirected to PluginAsyncSurrogate until eventually the regular code paths handle everything.

The one exception is scriptable objects: those are wrapped by AsyncNPObject for the lifetime of the plugin.

Note that PluginAsyncSurrogate doesn't intercept all plugin calls; just the ones that are needed during initialization. PluginAsyncSurrogate does do blocking, but avoids it until it is absolutely necessary for correctness. As implemented, this means that we can usually continue executing asynchronously until some js tries to call into the plugin.

Comment 36

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521712 [details] [diff] [review]
Part 15: Test updates

The problem with reftests and async init is that in the New World Order, the snapshot can't be taken until we know that the plugin has finished initializing. Since retrieving a property on the plugin's scriptable JS object causes us to block until it has completed, I've converted the relevant tests to read the test plugin's pluginFoundElement property and delaying the reftest snapshot until that time.

Comment 37

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

johns says he's still ok with reviewing, so I'll send this to him.
See comment 26 for a description of this patch.

Comment 38

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: dom/plugins/base Patches

I saw something I didn't like in this patch so I've revised it.

Comment 39

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

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

::: dom/plugins/base/nsJSNPRuntime.h
@@ +43,5 @@
>    const NPP mNpp;
>  
>    static NPObject *GetNewOrUsed(NPP npp, JSContext *cx,
>                                  JS::Handle<JSObject*> obj);
> +  static bool NP_HasOwnProperty(NPObject* npobj, NPIdentifier npid);

The NP_ prefix would suggest it is part of the NPAPI, so we should drop it here.

Comment 40

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521883 [details] [diff] [review]
Part 2: dom/plugins/base Patches

For the most part, this patch consists of changes that provide entry points for asynchronous callbacks to invoke plugin base code when they receive a result.

The other main change is to PluginDestructionGuard: In async init land we need to guard against plugin destruction while an async operation is running in the child process. This can't be done on the stack, so I removed the MOZ_STACK_CLASS attribute and added a special constructor for the asynchronous case. In this case, the guard must be inserted into the linked list such that it appears to be at the *bottom* of the stack instead of the top.

Comment 41

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521710 [details] [diff] [review]
Part 13: PluginAsyncSurrogate

Adding johns particularly for the scriptable object bits.

Comment 42

[John Schoenick [:johns]]

Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

moved the email on this account, stealing

Comment 43

[Jim Mathies [:jimm]]

Comment on attachment 8521695 [details] [diff] [review]
Part 4: IPDL changes

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

::: dom/plugins/ipc/PPluginInstance.ipdl
@@ +219,1 @@
>                       uint32_t length,

nit - indent

@@ +225,3 @@
>      returns (NPError rv,
>               uint16_t stype);
> +  async AsyncNPP_NewStream(PBrowserStream actor, nsCString mimeType, bool seekable);

here too, see below.

::: dom/plugins/ipc/PPluginModule.ipdl
@@ +43,5 @@
> +                        uint16_t aMode,
> +                        nsCString[] aNames,
> +                        nsCString[] aValues);
> +
> +  intr SyncNPP_New(PPluginInstance aActor)

what's the purpose of these match sync/async calls (SyncNPP_New, AsyncNPP_New)? I'm guessing we still use the sync call internally in response to the async request. I guess I'll get to that. some commenting here would be good with these pairs.

Comment 44

[Jim Mathies [:jimm]]

Comment on attachment 8521696 [details] [diff] [review]
Part 5: PluginModuleChild changes

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

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2002,5 @@
>  #endif
>  }
>  
> +/*
> +struct mozilla::plugins::NPPNewClosure

hmm, not in use? please remove.

@@ +2034,1 @@
>                                                      const nsCString& aMimeType,

nit - indent

Comment 45

[Jim Mathies [:jimm]]

Comment on attachment 8521698 [details] [diff] [review]
Part 6: PluginInstanceChild changes

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +237,5 @@
> +    }
> +
> +    NPP npp = GetNPP();
> +
> +    // FIXME/cjones: use SAFE_CALL stuff

since we have the chance, please remove this.

Comment 46

[Jim Mathies [:jimm]]

Comment on attachment 8521704 [details] [diff] [review]
Part 9: PluginModuleParent changes

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +249,5 @@
>  #endif
>  
> +    if (mInitOnAsyncConnect) {
> +        mInitOnAsyncConnect = false;
> +#if defined(XP_WIN) || defined(XP_OS2)

seriously doubt that we support os2 anymore, you can take this out.

@@ +1082,5 @@
> +    } \
> +    if (!i) { \
> +        return NPERR_GENERIC_ERROR; \
> +    } \
> +    return i->func; \

nit - I think I'd prefer the ending slash to be lined up on the nd, so the code is still readable.

@@ +1670,5 @@
> +#else
> +    if (mIsStartingAsync) {
> +        PluginAsyncSurrogate::NP_GetEntryPoints(pFuncs);
> +    }
> +    if (!mSubprocess->IsConnected()) {

curious, why does mac get skipped for this connection check?

::: dom/plugins/ipc/PluginModuleParent.h
@@ +89,5 @@
>      virtual ~PluginModuleParent();
>  
> +    bool RemovePendingSurrogate(const nsRefPtr<PluginAsyncSurrogate>& aSurrogate);
> +
> +    bool IsStartingAsync() const { return mIsStartingAsync; }

How long during the run is this valid? A comment here would be good.

@@ +90,5 @@
>  
> +    bool RemovePendingSurrogate(const nsRefPtr<PluginAsyncSurrogate>& aSurrogate);
> +
> +    bool IsStartingAsync() const { return mIsStartingAsync; }
> +    bool IsInitialized() const { return mNPInitialized; }

comment please.

Comment 47

[Jim Mathies [:jimm]]

Comment on attachment 8521706 [details] [diff] [review]
Part 10: PluginInstanceParent changes

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

::: dom/plugins/ipc/PluginInstanceParent.h
@@ +281,5 @@
>  
> +    virtual PluginAsyncSurrogate* GetAsyncSurrogate();
> +
> +    virtual PluginInstanceParent*
> +    GetInstance() { return this; }

nit - lets not wrap this

@@ +284,5 @@
> +    virtual PluginInstanceParent*
> +    GetInstance() { return this; }
> +
> +    static PluginInstanceParent* Cast(NPP instance,
> +                                   PluginAsyncSurrogate** aSurrogate = nullptr);

nit - indent

Comment 48

[Dave Garrett]

(In reply to Jim Mathies [:jimm] from comment #46)
> Comment on attachment 8521704 [details] [diff] [review]
> > +#if defined(XP_WIN) || defined(XP_OS2)
> 
> seriously doubt that we support os2 anymore, you can take this out.

See bug 969757. Yes, OS/2 support should not be in the tree.

If usage of XP_OS2 is at risk of sneaking back in, someone might want to file a new bug to get usage of it to produce an error instead.

Comment 49

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8521712 [details] [diff] [review]
Part 15: Test updates

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

::: dom/plugins/test/reftest/border-padding-1.html
@@ +1,5 @@
>  <!DOCTYPE html>
> +<html class="reftest-wait">
> +<head>
> +<script>
> +  function forceLoadPlugin() {

Hm, can we please use a shared |forceLoadPlugin(id, takeSnapshot=true)| or something similar?

::: layout/reftests/bugs/632423-1-ref.html
@@ +1,3 @@
>  <!DOCTYPE HTML>
> +<html>
> +<body>

This change is not required, is it?

Comment 50

[Jim Mathies [:jimm]]

aaron, could you post a rollup merged to tip for testing?

Comment 51

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Rollup patch for testing

Done. I've also pushed to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d4fa3f54173

Comment 52

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8521693 [details] [diff] [review]
Part 3: ipc/glue patches

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

::: ipc/glue/MessageChannel.h
@@ +339,5 @@
> +        mMonitor->AssertCurrentThreadOwns();
> +        return mIsWaitingForIncoming;
> +    }
> +
> +    class MOZ_STACK_CLASS SetWaitingForIncoming

Prefer this to have "Auto" in its name, like AutoSetWaitingForIncoming or AutoEnterWaitForIncoming.

@@ +355,5 @@
> +            mIsWaitingForIncoming = false;
> +        }
> +
> +    private:
> +        bool& mIsWaitingForIncoming;

Store a ref to the channel instead of the bool, makes reading/debugging easier.

@@ +646,5 @@
>      // Did we process an Interrupt out-call during this stack?  Only meaningful in
>      // ExitedCxxStack(), from which this variable is reset.
>      bool mSawInterruptOutMsg;
>  
> +    bool mIsWaitingForIncoming;

Can you put a comment here explaining when this is set and where/when it's okay to read it?

::: ipc/glue/WindowsMessageLoop.cpp
@@ +965,5 @@
>    if (!(mFlags & REQUIRE_DEFERRED_MESSAGE_PROTECTION)) {
>      return WaitForSyncNotify();
>    }
>  
> +  if (!InterruptStackDepth() && !mIsWaitingForIncoming) {

Should this be !AwaitingIncomingMessage()?

Comment 53

[Jim Mathies [:jimm]]

Comment on attachment 8521710 [details] [diff] [review]
Part 13: PluginAsyncSurrogate

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

::: dom/plugins/ipc/PluginAsyncSurrogate.cpp
@@ +390,5 @@
> +  mInstantiated = true;
> +}
> +
> +bool
> +PluginAsyncSurrogate::WaitForInit()

I'm a little concerned about the heavy use of this wait function. Some commenting here explaining what we're doing here and what the risks are would be good.

Comment 54

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 13: PluginAsyncSurrogate (r2)

(In reply to Jim Mathies [:jimm] from comment #53)
> Comment on attachment 8521710 [details] [diff] [review]
> Part 13: PluginAsyncSurrogate
> 
> Review of attachment 8521710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/ipc/PluginAsyncSurrogate.cpp
> @@ +390,5 @@
> > +  mInstantiated = true;
> > +}
> > +
> > +bool
> > +PluginAsyncSurrogate::WaitForInit()
> 
> I'm a little concerned about the heavy use of this wait function. Some
> commenting here explaining what we're doing here and what the risks are
> would be good.

I added some comments to the patch but I'll include them here as well:

/**
 * During asynchronous initialization it might be necessary to wait for the
 * plugin to complete its initialization. This typically occurs when the result
 * of a plugin call depends on the plugin being fully instantiated. For example,
 * if some JS calls into the plugin, the call must be executed synchronously to
 * preserve correctness.
 *
 * This function works by pumping the plugin's IPC channel for events until
 * initialization has completed.
 */

Essentially it is WaitForInit that preserves the semantics of plugin calls that we have not (or cannot be) made asynchronous. Note that WaitForInit does *not* spin the main event loop, but rather pumps the message channel for events until the plugin has initialized. Under the hood it used the same mechanism that intr calls use for waiting for their replies.

Comment 55

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: ipc/glue patches (r2)

(In reply to David Anderson [:dvander] from comment #52)
> Comment on attachment 8521693 [details] [diff] [review]
> Part 3: ipc/glue patches
> 
> Review of attachment 8521693 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/MessageChannel.h
> @@ +339,5 @@
> > +        mMonitor->AssertCurrentThreadOwns();
> > +        return mIsWaitingForIncoming;
> > +    }
> > +
> > +    class MOZ_STACK_CLASS SetWaitingForIncoming
> 
> Prefer this to have "Auto" in its name, like AutoSetWaitingForIncoming or
> AutoEnterWaitForIncoming.

Done. Renamed to AutoEnterWaitForIncoming.

> 
> @@ +355,5 @@
> > +            mIsWaitingForIncoming = false;
> > +        }
> > +
> > +    private:
> > +        bool& mIsWaitingForIncoming;
> 
> Store a ref to the channel instead of the bool, makes reading/debugging
> easier.

Done.

> 
> @@ +646,5 @@
> >      // Did we process an Interrupt out-call during this stack?  Only meaningful in
> >      // ExitedCxxStack(), from which this variable is reset.
> >      bool mSawInterruptOutMsg;
> >  
> > +    bool mIsWaitingForIncoming;
> 
> Can you put a comment here explaining when this is set and where/when it's
> okay to read it?

Done.

> 
> ::: ipc/glue/WindowsMessageLoop.cpp
> @@ +965,5 @@
> >    if (!(mFlags & REQUIRE_DEFERRED_MESSAGE_PROTECTION)) {
> >      return WaitForSyncNotify();
> >    }
> >  
> > +  if (!InterruptStackDepth() && !mIsWaitingForIncoming) {
> 
> Should this be !AwaitingIncomingMessage()?

Fixed.

Comment 56

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8524748 [details] [diff] [review]
Part 3: ipc/glue patches (r2)

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

::: ipc/glue/MessageChannel.h
@@ +647,5 @@
>      // ExitedCxxStack(), from which this variable is reset.
>      bool mSawInterruptOutMsg;
>  
> +    // Are we waiting on this channel for an incoming message? This is used
> +    // to implement WaitForIncomingMessage(). Must only be accessed while owning

Since ipc/glue is rather hairy, would be good to be more descriptive here: "This is used to indicate that the channel is blocking until it receives either a message of any kind, or a channel error. We can use this to detect whether the other side of a channel either successfully initialized or died in the process of initializing."

Comment 57

[John Schoenick [:johns]]

Comment on attachment 8521883 [details] [diff] [review]
Part 2: dom/plugins/base Patches

Sorry for the latency on this, just got my machine with mozilla stuff set back up, I can take a look at these tomorrow.

Comment 58

[Jim Mathies [:jimm]]

Comment on attachment 8521704 [details] [diff] [review]
Part 9: PluginModuleParent changes

I had a question about some mac code down below.

Comment 59

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

In this bug we need to be able to query whether the plugin element has a property without walking the protochain, so I'd like to add a NP_HasOwnProperty function to nsJSObjWrapper.

Comment 60

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521703 [details] [diff] [review]
Part 8: Plugin process launcher changes

These changes make launching of the plugin process asynchronous. A task is provided by the caller to be invoked when the launch has completed.

Comment 61

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8521883 [details] [diff] [review]
Part 2: dom/plugins/base Patches

For the most part, this patch consists of changes that provide entry points for asynchronous callbacks to invoke plugin base code when they receive a result.

The other main change is to PluginDestructionGuard: In async init land we need to guard against plugin destruction while an async operation is running in the child process. This can't be done on the stack, so I removed the MOZ_STACK_CLASS attribute and added a special constructor for the asynchronous case. In this case, the guard must be inserted into the linked list such that it appears to be at the *bottom* of the stack instead of the top.

Comment 62

[John Schoenick [:johns]]

Comment on attachment 8521883 [details] [diff] [review]
Part 2: dom/plugins/base Patches

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

Josh probably knows more about the PluginStream fixes here, but this looks good to me with notes below

::: dom/plugins/base/nsPluginHost.cpp
@@ -840,5 @@
> -  if (instance) {
> -    instanceOwner->CreateWidget();
> -
> -    // If we've got a native window, the let the plugin know about it.
> -    instanceOwner->CallSetWindow();

ObjectLoadingContent also has a CallSetWindow call to work around an adobe reader bug:
http://dxr.mozilla.org/mozilla-central/source/dom/base/nsObjectLoadingContent.cpp#832

If that second call will fail due to async init, we should check that this dooesn't re-introduce bug 870216

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +1241,5 @@
> +void
> +nsPluginInstanceOwner::NotifyHostAsyncInitFailed()
> +{
> +  nsCOMPtr<nsIObjectLoadingContent> content = do_QueryInterface(mContent);
> +  content->StopPluginInstance();

ObjectLoadingContent will attempt to restart its plugin when it finds it in a stopped state and e.g. a new frame is created. This should probably call something similar to nsObjectLoadingContent::PluginCrashed() that changes state to fallback.

Comment 63

[Josh Aas]

Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

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

I don't know much about this code, you might want to find someone who knows more to review. I'll give it r+ in the sense that I don't have any objections to it.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +2311,5 @@
> +nsJSObjWrapper::NP_HasOwnProperty(NPObject *npobj, NPIdentifier npid)
> +{
> +  NPP npp = NPPStack::Peek();
> +  dom::AutoJSAPI jsapi;
> +  if (NS_WARN_IF(!jsapi.InitWithLegacyErrorReporting(GetGlobalObject(npp)))) {

You might want to break this line up a bit. Can GetGlobalObject return null? If so, is it OK to pass null to InitWithLegacyErrorReporting?

::: dom/plugins/base/nsJSNPRuntime.h
@@ +43,5 @@
>    const NPP mNpp;
>  
>    static NPObject *GetNewOrUsed(NPP npp, JSContext *cx,
>                                  JS::Handle<JSObject*> obj);
> +  static bool NP_HasOwnProperty(NPObject* npobj, NPIdentifier npid);

Agreed

Comment 64

[Josh Aas]

Comment on attachment 8521883 [details] [diff] [review]
Part 2: dom/plugins/base Patches

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

r+ with comments, including John's, addressed. Thanks for doing this work!

Please get in touch with me to coordinate landing this and bug 1092630 around the same time. They're both big, risky patches and it'd be good to reduce conflicts.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +2793,5 @@
>  NS_IMETHODIMP
>  nsPluginInstanceOwner::CallSetWindow()
>  {
> +  if (!mWidgetCreationComplete) {
> +    // No widget yet, we can't run this code

Have you checked to make sure that this will work with windowless plugins? In general it's unfortunate to be checking something called "mWidgetCreationComplete" in a windowless plugin code path.

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
@@ -1129,5 @@
> -  if (!useLocalCache && mStreamType >= NP_ASFILE) {
> -    // check it out if this is not a file channel.
> -    nsCOMPtr<nsIFileChannel> fileChannel = do_QueryInterface(request);
> -    if (!fileChannel) {
> -        useLocalCache = true;

Too much indentation here.

Comment 65

[Johnny Stenback (:jst)]

Excellent work here guys. Aaron, looks like this is very close, when do you think this is ready to land?

Comment 66

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Johnny Stenback  (:jst, [e-mail]) from comment #65)
> Excellent work here guys. Aaron, looks like this is very close, when do you
> think this is ready to land?

Imminently! I'm retesting with e10s because I've had to deal with some merge conflicts there.

Comment 68

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #46)
> Comment on attachment 8521704 [details] [diff] [review]
> Part 9: PluginModuleParent changes

All comments addressed.

> 
> Review of attachment 8521704 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +1670,5 @@
> > +#else
> > +    if (mIsStartingAsync) {
> > +        PluginAsyncSurrogate::NP_GetEntryPoints(pFuncs);
> > +    }
> > +    if (!mSubprocess->IsConnected()) {
> 
> curious, why does mac get skipped for this connection check?

Mac and Windows differ in the order that NP_GetEntryPoints and NP_Initialize are called. In the mac's case, the connection check happens in NP_Initialize.

Comment 69

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: nsJSNPRuntime patch (r2)

Debitrotted, carrying forward r+.

Comment 70

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: dom/plugins/base Patches (r2)

Debitrotted, carrying forward r+.

Comment 71

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: ipc/glue patches (r3)

Debitrotted. Carrying forward r+.

Comment 72

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 4: IPDL changes (r2)

Debitrotted, comments addressed. Carrying forward r+.

Comment 73

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: PluginModuleChild changes (r2)

Carrying forward r+

Comment 74

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 6: PluginInstanceChild changes

Comment 75

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 7: PluginDataResolver (r2)

Comment 76

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 8: Plugin process launcher changes (r2)

Comment 77

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 9: PluginModuleParent changes (r2)

Debitrotted, merge conflicts resolved, carrying forward r+.

Comment 78

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 10: PluginInstanceParent changes (r2)

Comment 79

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 11: IPC Browser stream changes (r2)

Comment 80

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 12: PluginScriptableObject changes (r2)

Comment 81

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 13: PluginAsyncSurrogate (r3)

Comment 82

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 14: Build system updates (r2)

Comment 83

[(No longer employed by Mozilla) Aaron Klotz]

All comments were addressed with the exception of the remarks about parts 2 and 15: I'm going to address those in follow-up bugs since this feature is preffed off for now.

Let's see if these stick:

https://hg.mozilla.org/integration/mozilla-inbound/rev/18dad6d2e7cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/dabc69b0b09a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba058cc7a1b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4244d662787c
https://hg.mozilla.org/integration/mozilla-inbound/rev/713799a7afe4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b68f8f72ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e69875e3667
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c1f006a03bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd57abaf2b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/404f59f86edc
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe21b6b789ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2cf239e8572
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f587697ae63
https://hg.mozilla.org/integration/mozilla-inbound/rev/43819af59ca5

Comment 84

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/76d1641cf90b for m-e10s-3 and c-e10s hangs or crashes or whatever they actually turn out to be.

Comment 85

[(No longer employed by Mozilla) Aaron Klotz]

Broken by changes made in bug 1109883. I'm not going to be able to fix this until after Christmas.

Comment 86

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: PluginModuleParent changes (r3)

Fixed issue that caused backout. Carrying forward r+.

Comment 87

[(No longer employed by Mozilla) Aaron Klotz]

Here we go again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/187fab61f428
https://hg.mozilla.org/integration/mozilla-inbound/rev/70509d064c20
https://hg.mozilla.org/integration/mozilla-inbound/rev/07f4a782ff63
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de6c193d867
https://hg.mozilla.org/integration/mozilla-inbound/rev/9382b0d2d230
https://hg.mozilla.org/integration/mozilla-inbound/rev/5287f9dfdb8c
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e07ce94eb7
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6ac47db856
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4c2bf5ec09
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5bf5f84932
https://hg.mozilla.org/integration/mozilla-inbound/rev/8534359e8965
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0b1cab268db
https://hg.mozilla.org/integration/mozilla-inbound/rev/57d34003ea77
https://hg.mozilla.org/integration/mozilla-inbound/rev/a656ced1d0fb

Comment 88

[Nigel Babu [:nigelb]]

There's a non-unified bustage in the new patch.

Comment 89

[Nigel Babu [:nigelb]]

Will you be able to fix or shall I back it out?

Comment 90

[Nigel Babu [:nigelb]]

I've just landed a fix instead with glandium's help

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcdfb7d47af

Comment 91

[Nigel Babu [:nigelb]]

Another bustage fix for namespace issues

https://hg.mozilla.org/integration/mozilla-inbound/rev/d850c0d21fed

If this doesn't work, I've advised Tomcat, the next sheriff, to back the bug out.

Comment 92

[Nigel Babu [:nigelb]]

This still broke. We're still waiting to see if my try run with a fix returns a green: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=efcad73ef57c

Comment 93

[(No longer employed by Mozilla) Aaron Klotz]

This should fix it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef3f759cc0b4

I can't wait until we don't have to do this anymore. *sigh*

Comment 94

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/187fab61f428
https://hg.mozilla.org/mozilla-central/rev/70509d064c20
https://hg.mozilla.org/mozilla-central/rev/07f4a782ff63
https://hg.mozilla.org/mozilla-central/rev/5de6c193d867
https://hg.mozilla.org/mozilla-central/rev/9382b0d2d230
https://hg.mozilla.org/mozilla-central/rev/5287f9dfdb8c
https://hg.mozilla.org/mozilla-central/rev/23e07ce94eb7
https://hg.mozilla.org/mozilla-central/rev/eb6ac47db856
https://hg.mozilla.org/mozilla-central/rev/8a4c2bf5ec09
https://hg.mozilla.org/mozilla-central/rev/ce5bf5f84932
https://hg.mozilla.org/mozilla-central/rev/8534359e8965
https://hg.mozilla.org/mozilla-central/rev/e0b1cab268db
https://hg.mozilla.org/mozilla-central/rev/57d34003ea77
https://hg.mozilla.org/mozilla-central/rev/a656ced1d0fb
https://hg.mozilla.org/mozilla-central/rev/8bcdfb7d47af
https://hg.mozilla.org/mozilla-central/rev/d850c0d21fed
https://hg.mozilla.org/mozilla-central/rev/ef3f759cc0b4

Comment 95

[Nigel Babu [:nigelb]]

Thank you :)