Asynchronous Initialization of Out-of-process Plugins

RESOLVED FIXED in mozilla37

Status

()

Core
Plug-ins
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla37
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(16 attachments, 19 obsolete attachments)

29.89 KB, patch
Details | Diff | Splinter Review
211.62 KB, patch
Details | Diff | Splinter Review
5.35 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
20.99 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
7.54 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
4.70 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
11.44 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
12.03 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.07 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
11.09 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
12.32 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
9.97 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
7.76 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
32.83 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
1.33 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
62.24 KB, patch
aklotz
: review+
Details | Diff | Splinter Review
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.
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?
Flags: needinfo?(jschoenick)
(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
Flags: needinfo?(jschoenick)
(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

3 years ago
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.
(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.
Duplicate of this bug: 522573
Duplicate of this bug: 517962
(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.
Created attachment 8451466 [details] [diff] [review]
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 :-)
Depends on: 1045178
Depends on: 1049751

Comment 10

3 years ago
What's the status of this feature?
Created attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch
Created attachment 8521691 [details] [diff] [review]
Part 2: dom/plugins/base Patches
Created attachment 8521693 [details] [diff] [review]
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.
Attachment #8521693 - Flags: review?(dvander)
Created attachment 8521695 [details] [diff] [review]
Part 4: IPDL changes
Created attachment 8521696 [details] [diff] [review]
Part 5: PluginModuleChild changes
Created attachment 8521698 [details] [diff] [review]
Part 6: PluginInstanceChild changes
Created attachment 8521700 [details] [diff] [review]
Part 7: PluginDataResolver
Created attachment 8521703 [details] [diff] [review]
Part 8: Plugin process launcher changes
Created attachment 8521704 [details] [diff] [review]
Part 9: PluginModuleParent changes
Created attachment 8521706 [details] [diff] [review]
Part 10: PluginInstanceParent changes
Created attachment 8521707 [details] [diff] [review]
Part 11: IPC Browser stream changes
Created attachment 8521708 [details] [diff] [review]
Part 12: PluginScriptableObject changes
Created attachment 8521710 [details] [diff] [review]
Part 13: PluginAsyncSurrogate
Attachment #8451466 - Attachment is obsolete: true
Created attachment 8521711 [details] [diff] [review]
Part 14: Build system updates
Created attachment 8521712 [details] [diff] [review]
Part 15: Test updates
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.
Attachment #8521690 - Flags: review?(jst)
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.
Attachment #8521695 - Flags: review?(jmathies)
Comment on attachment 8521696 [details] [diff] [review]
Part 5: PluginModuleChild changes

These changes are needed to handle the revised IPDL in the previous patch.
Attachment #8521696 - Flags: review?(jmathies)
Comment on attachment 8521698 [details] [diff] [review]
Part 6: PluginInstanceChild changes

Changes to PluginInstanceChild to handle the revised IPC.
Attachment #8521698 - Flags: review?(jmathies)
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.
Attachment #8521700 - Flags: review?(jmathies)
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.
Attachment #8521703 - Flags: review?(benjamin)
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.
Attachment #8521704 - Flags: review?(jmathies)
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.
Attachment #8521706 - Flags: review?(jmathies)
Attachment #8521707 - Flags: review?(jmathies)
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.
Attachment #8521708 - Flags: review?(jmathies)
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.
Attachment #8521710 - Flags: review?(jmathies)
Attachment #8521711 - Flags: review?(jmathies)
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.
Attachment #8521712 - Flags: review?(georg.fritzsche)
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.
Attachment #8521690 - Flags: review?(jst) → review?(Zlixar)
Created attachment 8521883 [details] [diff] [review]
Part 2: dom/plugins/base Patches

I saw something I didn't like in this patch so I've revised it.
Attachment #8521691 - Attachment is obsolete: true
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 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.
Attachment #8521883 - Flags: review?(Zlixar)
Comment on attachment 8521710 [details] [diff] [review]
Part 13: PluginAsyncSurrogate

Adding johns particularly for the scriptable object bits.
Attachment #8521710 - Flags: review?(Zlixar)
Comment on attachment 8521690 [details] [diff] [review]
Part 1: nsJSNPRuntime patch

moved the email on this account, stealing
Attachment #8521690 - Flags: review?(Zlixar) → review?(john)
Attachment #8521710 - Flags: review?(Zlixar) → review?(john)
Attachment #8521883 - Flags: review?(Zlixar) → review?(john)

Comment 43

3 years ago
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.
Attachment #8521695 - Flags: review?(jmathies) → review+

Comment 44

3 years ago
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
Attachment #8521696 - Flags: review?(jmathies) → review+

Comment 45

3 years ago
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.
Attachment #8521698 - Flags: review?(jmathies) → review+

Updated

3 years ago
Attachment #8521700 - Flags: review?(jmathies) → review+

Comment 46

3 years ago
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

3 years ago
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
Attachment #8521706 - Flags: review?(jmathies) → review+

Comment 48

3 years ago
(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 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?
Attachment #8521712 - Flags: review?(georg.fritzsche)

Comment 50

3 years ago
aaron, could you post a rollup merged to tip for testing?
Flags: needinfo?
Created attachment 8523094 [details] [diff] [review]
Rollup patch for testing

Done. I've also pushed to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d4fa3f54173
Flags: needinfo?
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()?
Attachment #8521693 - Flags: review?(dvander)

Updated

3 years ago
Attachment #8521707 - Flags: review?(jmathies) → review+

Updated

3 years ago
Attachment #8521711 - Flags: review?(jmathies) → review+

Updated

3 years ago
Attachment #8521708 - Flags: review?(jmathies) → review+

Comment 53

3 years ago
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.
Created attachment 8524745 [details] [diff] [review]
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.
Attachment #8521710 - Attachment is obsolete: true
Attachment #8521710 - Flags: review?(john)
Attachment #8521710 - Flags: review?(jmathies)
Attachment #8524745 - Flags: review?(jmathies)
Created attachment 8524748 [details] [diff] [review]
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.
Attachment #8521693 - Attachment is obsolete: true
Attachment #8524748 - Flags: review?(dvander)

Updated

3 years ago
Attachment #8524745 - Flags: review?(jmathies) → review+
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."
Attachment #8524748 - Flags: review?(dvander) → review+
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

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

I had a question about some mac code down below.
Attachment #8521704 - Flags: review?(jmathies) → review+
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.
Attachment #8521690 - Flags: review?(john) → review?(joshmoz)
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.
Attachment #8521703 - Flags: review?(benjamin) → review?(joshmoz)
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.
Attachment #8521883 - Flags: review?(john) → review?(joshmoz)
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

3 years ago
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
Attachment #8521690 - Flags: review?(joshmoz) → review+

Updated

3 years ago
Attachment #8521703 - Flags: review?(joshmoz) → review+

Comment 64

3 years ago
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.
Attachment #8521883 - Flags: review?(joshmoz) → review+
Excellent work here guys. Aaron, looks like this is very close, when do you think this is ready to land?
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) 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.

Updated

3 years ago
Duplicate of this bug: 416716
(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.
Created attachment 8541333 [details] [diff] [review]
Part 1: nsJSNPRuntime patch (r2)

Debitrotted, carrying forward r+.
Attachment #8521690 - Attachment is obsolete: true
Attachment #8541333 - Flags: review+
Created attachment 8541334 [details] [diff] [review]
Part 2: dom/plugins/base Patches (r2)

Debitrotted, carrying forward r+.
Attachment #8521883 - Attachment is obsolete: true
Attachment #8541334 - Flags: review+
Created attachment 8541336 [details] [diff] [review]
Part 3: ipc/glue patches (r3)

Debitrotted. Carrying forward r+.
Attachment #8524748 - Attachment is obsolete: true
Attachment #8541336 - Flags: review+
Created attachment 8541337 [details] [diff] [review]
Part 4: IPDL changes (r2)

Debitrotted, comments addressed. Carrying forward r+.
Attachment #8521695 - Attachment is obsolete: true
Attachment #8541337 - Flags: review+
Created attachment 8541338 [details] [diff] [review]
PluginModuleChild changes (r2)

Carrying forward r+
Attachment #8521696 - Attachment is obsolete: true
Attachment #8541338 - Flags: review+
Created attachment 8541340 [details] [diff] [review]
Part 6: PluginInstanceChild changes
Attachment #8521698 - Attachment is obsolete: true
Attachment #8541340 - Flags: review+
Created attachment 8541341 [details] [diff] [review]
Part 7: PluginDataResolver (r2)
Attachment #8521700 - Attachment is obsolete: true
Attachment #8541341 - Flags: review+
Created attachment 8541342 [details] [diff] [review]
Part 8: Plugin process launcher changes (r2)
Attachment #8521703 - Attachment is obsolete: true
Attachment #8541342 - Flags: review+
Created attachment 8541343 [details] [diff] [review]
Part 9: PluginModuleParent changes (r2)

Debitrotted, merge conflicts resolved, carrying forward r+.
Attachment #8521704 - Attachment is obsolete: true
Attachment #8541343 - Flags: review+
Created attachment 8541344 [details] [diff] [review]
Part 10: PluginInstanceParent changes (r2)
Attachment #8521706 - Attachment is obsolete: true
Attachment #8541344 - Flags: review+
Created attachment 8541345 [details] [diff] [review]
Part 11: IPC Browser stream changes (r2)
Attachment #8521707 - Attachment is obsolete: true
Attachment #8541345 - Flags: review+
Created attachment 8541346 [details] [diff] [review]
Part 12: PluginScriptableObject changes (r2)
Attachment #8521708 - Attachment is obsolete: true
Attachment #8541346 - Flags: review+
Created attachment 8541347 [details] [diff] [review]
Part 13: PluginAsyncSurrogate (r3)
Attachment #8524745 - Attachment is obsolete: true
Attachment #8541347 - Flags: review+
Created attachment 8541348 [details] [diff] [review]
Part 14: Build system updates (r2)
Attachment #8521711 - Attachment is obsolete: true
Attachment #8541348 - Flags: review+
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
Depends on: 1115436
Depends on: 1115437
Depends on: 1115438
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.
Broken by changes made in bug 1109883. I'm not going to be able to fix this until after Christmas.
Created attachment 8542306 [details] [diff] [review]
PluginModuleParent changes (r3)

Fixed issue that caused backout. Carrying forward r+.
Attachment #8541343 - Attachment is obsolete: true
Attachment #8542306 - Flags: review+
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
Depends on: 1112827
There's a non-unified bustage in the new patch.
Will you be able to fix or shall I back it out?
Flags: needinfo?(aklotz)
I've just landed a fix instead with glandium's help

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcdfb7d47af
Flags: needinfo?(aklotz)
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.
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
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*
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Thank you :)
Blocks: 1116806
No longer depends on: 1115437
No longer depends on: 1115436

Updated

3 years ago
Depends on: 1116825
No longer depends on: 1116825
Depends on: 1117883
Duplicate of this bug: 1117964
Depends on: 1125128
No longer depends on: 1125128
Depends on: 1156861

Updated

2 years ago
Depends on: 1187724
No longer depends on: 1187724
Depends on: 1193520

Updated

2 years ago
Depends on: 1194600
No longer depends on: 1194600
No longer depends on: 1193520
Blocks: 1352575
You need to log in before you can comment on or make changes to this bug.