1135491 - Intermittent browser_pluginnotification.js | uncaught exception - Error: Permission denied to access property "pluginFoundWindow" at :0

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[(No longer employed by Mozilla) Aaron Klotz]

22:03:19     INFO -  836 INFO TEST-PASS | browser/base/content/test/plugins/browser_pluginnotification.js | Test 12d, Should have a click-to-play notification
22:03:19     INFO -  837 INFO TEST-PASS | browser/base/content/test/plugins/browser_pluginnotification.js | Test 12d, Test plugin should be activated
22:03:19     INFO -  838 INFO TEST-PASS | browser/base/content/test/plugins/browser_pluginnotification.js | Test 12d, Second Test plugin (A) should not be activated
22:03:19     INFO -  839 INFO TEST-PASS | browser/base/content/test/plugins/browser_pluginnotification.js | Test 12d, Second Test plugin (B) should not be activated
22:03:19     INFO -  ++DOMWINDOW == 94 (000000D571B7EC00) [pid = 2736] [serial = 227] [outer = 000000D508B03400]
22:03:19     INFO -  840 INFO checking window state
22:03:19     INFO -  841 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_pluginnotification.js | uncaught exception - Error: Permission denied to access property "pluginFoundWindow" at :0
22:03:19     INFO -  Stack trace:
22:03:19     INFO -  chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1476
22:03:19     INFO -  null:null:0
22:03:19     INFO -  JavaScript error: , line 0: Error: Permission denied to access property "pluginFoundWindow"
22:03:19     INFO -  842 INFO Console message: [JavaScript Error: "Error: Permission denied to access property "pluginFoundWindow""]

Comment 1

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=6860637
repository: mozilla-inbound
start_time: 2015-02-22T00:43:14
who: aklotz[at]mozilla[dot]com
machine: t-w864-ix-086
buildname: Windows 8 64-bit mozilla-inbound debug test mochitest-browser-chrome-1
revision: c07df64140ae

836 INFO TEST-UNEXPECTED-FAIL | browser/base/content/test/plugins/browser_pluginnotification.js | uncaught exception - Error: Permission denied to access property "pluginFoundWindow" at :0
Return code: 1

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

I think I've got this nailed down.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=661ec1f3e848

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add functions to notify plugin code when a destroy is pending

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Notify a plugin when its destruction is being deferred

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 8570660 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending

Bobby, can you have a quick look at the exception handling and clearing code here? Thanks.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 8570662 [details] [diff] [review]
Part 3: Notify a plugin when its destruction is being deferred

There are several places where we stop plugins unfortunately. Is this the only place where we need to notify about a pending destroy?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Johnny Stenback  (:jst, [e-mail]) from comment #7)
> Comment on attachment 8570662 [details] [diff] [review]
> Part 3: Notify a plugin when its destruction is being deferred
> 
> There are several places where we stop plugins unfortunately. Is this the
> only place where we need to notify about a pending destroy?

Inside the plugin code itself, I've added code that does the right thing whenever NPP_Destroy() is invoked. This is a special case since the NPP_Destroy itself is being deferred. AFAIK this is the only place where the latter occurs.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 8570660 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending

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

I don't know this code at all, deferring to bholly.

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 8570662 [details] [diff] [review]
Part 3: Notify a plugin when its destruction is being deferred

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

::: dom/base/nsObjectLoadingContent.cpp
@@ +2885,5 @@
>        && !aInstanceOwner->MatchPluginName("XStandard plugin")
>        && !aInstanceOwner->MatchPluginName("CMISS Zinc Plugin")
>  #endif
>        ) {
> +    aInstanceOwner->NotifyDestroyPending();

nit - a comment here explaining this call might be good.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8570660 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending

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

Sorry for the terrible lag here - I've been swamped. Followups will be quick, I promise. :-)

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +121,5 @@
>  
>  // Helper class that reports any JS exceptions that were thrown while
>  // the plugin executed JS.
>  
>  class AutoJSExceptionReporter

Please mark this MOZ_STACK_CLASS

@@ +124,5 @@
>  
>  class AutoJSExceptionReporter
>  {
>  public:
> +  AutoJSExceptionReporter(dom::AutoJSAPI& aJsapi, nsJSObjWrapper* aWrapper)

The convention is to call AutoJSAPI& args just |jsapi|.

@@ +125,5 @@
>  class AutoJSExceptionReporter
>  {
>  public:
> +  AutoJSExceptionReporter(dom::AutoJSAPI& aJsapi, nsJSObjWrapper* aWrapper)
> +    : mCx(aJsapi.cx())

We shouldn't be storing a JSContext (which are a concept that will go away). We should store the AutoJSAPI&.

@@ +131,4 @@
>    {
> +    if (mIsDestroyPending) {
> +      aJsapi.TakeOwnershipOfErrorReporting();
> +    }

We're moving to a world where the embedding _always_ manages the exception. So for new code like this, we should call TakeOwnershipOfErrorReporting unconditionally.

@@ +139,5 @@
> +    if (mIsDestroyPending) {
> +      JS_ClearPendingException(mCx);
> +    } else {
> +      JS_ReportPendingException(mCx);
> +    }

here we should doing one of three things:

jsapi.ClearException - squelches
jsapi.StealException - grab the exception and do something custom with it
nothing - the AutoJSAPI destructor will take care of reporting the exception.

@@ +1129,5 @@
>    }
>  
> +  // If we're running out-of-process and initializing asynchronously, and if
> +  // the plugin has been asked to destroy itself during initialization,
> +  // don't return any new NPObjects.

I don't know what the deal with this part is - do you want me to review it?

@@ +2054,5 @@
> +    for (JSObjWrapperTable::Enum e(sJSObjWrappers); !e.empty(); e.popFront()) {
> +      nsJSObjWrapper *npobj = e.front().value();
> +      MOZ_ASSERT(npobj->_class == &nsJSObjWrapper::sJSObjWrapperNPClass);
> +      if (npobj->mNpp == npp) {
> +        npobj->mDestroyPending = true;

Same here - I don't grok the significance of the mDestroyPending stuff (a plugin-y person should probably look at that). I'm just reviewing the exception semantics.

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending (r2)

(In reply to Bobby Holley (:bholley) from comment #11)
> Comment on attachment 8570660 [details] [diff] [review]
> Part 2: Changes to ensure that JS exceptions are suppressed when a destroy
> is pending
> 
> Review of attachment 8570660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the terrible lag here - I've been swamped. Followups will be
> quick, I promise. :-)
> 
> ::: dom/plugins/base/nsJSNPRuntime.cpp
> @@ +121,5 @@
> >  
> >  // Helper class that reports any JS exceptions that were thrown while
> >  // the plugin executed JS.
> >  
> >  class AutoJSExceptionReporter
> 
> Please mark this MOZ_STACK_CLASS

Done.

> 
> @@ +124,5 @@
> >  
> >  class AutoJSExceptionReporter
> >  {
> >  public:
> > +  AutoJSExceptionReporter(dom::AutoJSAPI& aJsapi, nsJSObjWrapper* aWrapper)
> 
> The convention is to call AutoJSAPI& args just |jsapi|.

Done.

> 
> @@ +125,5 @@
> >  class AutoJSExceptionReporter
> >  {
> >  public:
> > +  AutoJSExceptionReporter(dom::AutoJSAPI& aJsapi, nsJSObjWrapper* aWrapper)
> > +    : mCx(aJsapi.cx())
> 
> We shouldn't be storing a JSContext (which are a concept that will go away).
> We should store the AutoJSAPI&.

Done. Is calling the member variable |mJsapi| OK?

> 
> @@ +131,4 @@
> >    {
> > +    if (mIsDestroyPending) {
> > +      aJsapi.TakeOwnershipOfErrorReporting();
> > +    }
> 
> We're moving to a world where the embedding _always_ manages the exception.
> So for new code like this, we should call TakeOwnershipOfErrorReporting
> unconditionally.
> 

Done.

> @@ +139,5 @@
> > +    if (mIsDestroyPending) {
> > +      JS_ClearPendingException(mCx);
> > +    } else {
> > +      JS_ReportPendingException(mCx);
> > +    }
> 
> here we should doing one of three things:
> 
> jsapi.ClearException - squelches
> jsapi.StealException - grab the exception and do something custom with it
> nothing - the AutoJSAPI destructor will take care of reporting the exception.

Fixed. It calls ClearException if a plugin destroy is pending, otherwise we leave it to the AutoJSAPI destructor.

> 
> @@ +1129,5 @@
> >    }
> >  
> > +  // If we're running out-of-process and initializing asynchronously, and if
> > +  // the plugin has been asked to destroy itself during initialization,
> > +  // don't return any new NPObjects.
> 
> I don't know what the deal with this part is - do you want me to review it?

I'll have a plugin peer look into that stuff.

> 
> @@ +2054,5 @@
> > +    for (JSObjWrapperTable::Enum e(sJSObjWrappers); !e.empty(); e.popFront()) {
> > +      nsJSObjWrapper *npobj = e.front().value();
> > +      MOZ_ASSERT(npobj->_class == &nsJSObjWrapper::sJSObjWrapperNPClass);
> > +      if (npobj->mNpp == npp) {
> > +        npobj->mDestroyPending = true;
> 
> Same here - I don't grok the significance of the mDestroyPending stuff (a
> plugin-y person should probably look at that). I'm just reviewing the
> exception semantics.

Ditto.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8574852 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending (r2)

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

Looks good. r=bholley on the exception handling pieces.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8574852 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending (r2)

Josh, would you mind reviewing this from a plugin perspective? bholley has already r+'s the jsapi bits.

The problem in this bug is that when plugin initialization is asynchronous, it is possible for the user to have navigated away from the page while the plugin is still initializing. If the plugin tries to set a property (for example pluginFoundWindow in the test plugin) after the user has already navigated away, exceptions might be thrown.

My solution is to set a boolean when a plugin destroy is pending but not yet able to execute. We only throw the exceptions when the plugin destruction is not pending, otherwise we suppress them.

Comment 15

[(No longer employed by Mozilla) Aaron Klotz]

Please see comment 8.

Comment 16

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8570662 [details] [diff] [review]
Part 3: Notify a plugin when its destruction is being deferred

Upon further analysis and testing, this patch is unnecessary.

Comment 17

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14f614271c2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c7d7520c3a

Comment 18

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8570659 [details] [diff] [review]
Part 1: Add functions to notify plugin code when a destroy is pending

Approval Request Comment
[Feature/regressing bug #]: Asynchronous plugin initialization
[User impact if declined]: Feature cannot ride the 38 train
[Describe test coverage new/current, TreeHerder]: When the dom.ipc.plugins.asyncInit pref is turned on, this bug is no longer triggered.
[Risks and why]: Low, fix looks good and feature can be preffed off if need be
[String/UUID change made/needed]: None

Comment 19

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8574852 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending (r2)

Approval Request Comment
[Feature/regressing bug #]: Asynchronous plugin initialization
[User impact if declined]: Feature cannot ride the 38 train
[Describe test coverage new/current, TreeHerder]: When the dom.ipc.plugins.asyncInit pref is turned on, this bug is no longer triggered.
[Risks and why]: Low, fix looks good and feature can be preffed off if need be
[String/UUID change made/needed]: None

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/14f614271c2e
https://hg.mozilla.org/mozilla-central/rev/b9c7d7520c3a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/538258785927
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1dc2c9553d4