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

RESOLVED FIXED in Firefox 38

Status

()

Core
Plug-ins
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

({intermittent-failure})

Trunk
mozilla39
x86_64
Windows 8
intermittent-failure
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37 unaffected, firefox38 fixed, firefox39 fixed, firefox-esr31 unaffected)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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 hidden (Treeherder Robot)
I think I've got this nailed down.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=661ec1f3e848
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Created attachment 8570659 [details] [diff] [review]
Part 1: Add functions to notify plugin code when a destroy is pending
Attachment #8570659 - Flags: review?(jmathies)
Created attachment 8570660 [details] [diff] [review]
Part 2: Changes to ensure that JS exceptions are suppressed when a destroy is pending
Attachment #8570660 - Flags: review?(jmathies)
Created attachment 8570662 [details] [diff] [review]
Part 3: Notify a plugin when its destruction is being deferred
Attachment #8570662 - Flags: review?(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.
Attachment #8570660 - Flags: review?(bobbyholley)
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?
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) 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

2 years ago
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.
Attachment #8570660 - Flags: review?(jmathies)

Updated

2 years ago
Attachment #8570659 - Flags: review?(jmathies) → review+

Comment 10

2 years ago
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 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.
Attachment #8570660 - Flags: review?(bobbyholley) → review-
Created attachment 8574852 [details] [diff] [review]
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.
Attachment #8570660 - Attachment is obsolete: true
Attachment #8574852 - Flags: review?(bobbyholley)
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.
Attachment #8574852 - Flags: review?(bobbyholley) → review+
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.
Attachment #8574852 - Flags: review?(joshmoz)

Updated

2 years ago
Attachment #8574852 - Flags: review?(joshmoz) → review+
Please see comment 8.
Flags: needinfo?(jst)
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.
Flags: needinfo?(jst)
Attachment #8570662 - Flags: review?(jst)
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f614271c2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c7d7520c3a
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
Attachment #8570659 - Flags: approval-mozilla-aurora?
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
Attachment #8574852 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/14f614271c2e
https://hg.mozilla.org/mozilla-central/rev/b9c7d7520c3a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
status-firefox37: --- → unaffected
status-firefox38: --- → affected
status-firefox-esr31: --- → unaffected
Attachment #8574852 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8570659 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/538258785927
https://hg.mozilla.org/releases/mozilla-aurora/rev/b1dc2c9553d4
status-firefox38: affected → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.