Markus Hartung Assignee
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.71 Safari/537.36

Steps to reproduce:

Run the src/main/java/PopupTest.java from
https://github.com/markus-hartung-avira/marionette-popup-bug

The test does the following:
1. Start a firefox session with an extension installed
2. Click the extension icon which opens the popup
3. Execute a script (in popup context)
4. Close the popup
5. Execute another script (in default chrome context)

I originally reported this in bug 1366988, but the topic of the bug got changed to another issue mentioned in the report.



Actual results:

The test fails in step 5 due to the fact that in https://hg.mozilla.org/mozilla-central/file/44e41de60c48/testing/marionette/evaluate.js#l428 sb is a sandbox that was created in the context of the popup and when trying to access sb.window it will throw a TypeError: can't access dead object



Expected results:

Script should have been executed

	Markus Hartung Assignee
	Comment 1 • 7 years ago

I tested this on 53, 54 and 55

	Markus Hartung Assignee
	Comment 2 • 7 years ago

when previous sandbox is no longer available

	Markus Hartung Assignee
	Comment 3 • 7 years ago

when previous sandbox is no longer available

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 4 • 7 years ago

Thank you Markus for picking this up. Would you mind to upload the patch via mozreview? It would help us a lot to finally get it landed. You can read about the steps here:

https://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/commits.html#mozreview-commits

Thanks.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago

Comment on attachment 8871798 [details] [diff] [review]
Fix "can't access dead object" in Sandboxes.get

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

I think you’re on the right path here, but I think the patch can be simplified.  You can probably use Cu.isDeadWrapper(sb.window) to check if the object is dead.  In that case, I think I’d prefer the function to be called again as get(name, true) to force a refresh of the sandbox, instead of relying on the implicit sbWindow check you currently have.

::: testing/marionette/evaluate.js
@@ +428,5 @@
> +      let sbWindow;
> +      try {
> +        sbWindow = sb.window;
> +      } catch (e) {
> +        if (e.message == "can't access dead object") {

Drop the try…catch statement in favour of Cu.isDeadWrapper(sb.window).

@@ +429,5 @@
> +      try {
> +        sbWindow = sb.window;
> +      } catch (e) {
> +        if (e.message == "can't access dead object") {
> +          sbWindow = null

Call this function again as get(name, true) instead of relying on the sbWindow != this.window_ test below.  That way, the check will be explicit.

	Markus Hartung Assignee
	Comment 6 • 7 years ago

I tried your suggestion, but the problem is that this will still throw an error trying to pass sb.window to Cu.isDeadWrapper, I also tested to call Cu.isDeadWrapper on sb itself, but this returns false.


> get(name = "default", fresh = false) {
>    logger.debug('get', name, fresh)
>    let sb = this.boxes_.get(name);
>    if (sb) {
>      logger.debug('has sandbox');
>      logger.debug('sb dead', Cu.isDeadWrapper(sb));
>      try {
>        logger.debug('sb.window dead', Cu.isDeadWrapper(sb.window));
>      } catch (e) {
>        logger.debug(e);
>        fresh = true;
>      }
>      if (fresh || sb.window != this.window_) {

this will lead to

> 1496328324175	Marionette	DEBUG	get: default
> 1496328324176	Marionette	DEBUG	has sandbox
> 1496328324176	Marionette	DEBUG	sb dead: false
> 1496328324176	Marionette	DEBUG	TypeError: can't access dead object No traceback available
> 1496328324177	Marionette	DEBUG	get: default

Any idea why the Cu.isDeadWrapper(sb) would return false but accessing window doesn't work?

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 7 years ago

(In reply to Markus Hartung from comment #6)
> I tried your suggestion, but the problem is that this will still throw an
> error trying to pass sb.window to Cu.isDeadWrapper, I also tested to call
> Cu.isDeadWrapper on sb itself, but this returns false.

Sorry for leading you astray.  I see Cu.isDeadWrapper is used throughout
Gecko when Cu.nukeSandbox(sb) is called.  It appears calling a property on
the sandbox itself will still cause a TypeError, as we can seen here:

    https://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/js/xpconnect/tests/unit/test_nuke_sandbox.js#35-45

I think the sanest thing to do here is to introduce a new function in on
the evaluate module that returns a boolean based on whether it throws,
similar to your original patch:

> evaluate.isDead = function (obj, prop) {
>   try {
>     obj[prop];
>   } catch (e if e instanceof TypeError && e.message.includes("dead object")) {
>     return true;
>   }
>   return false;
> };

Sorry for this double work.

	Markus Hartung Assignee
	Comment 8 • 7 years ago

I will follow your suggestion, however I don't understand why Cu.isDeadWrapper(sb) doesn't return true because effectively the sandbox is dead, isn't it?

	Markus Hartung Assignee
	Comment 9 • 7 years ago

https://reviewboard.mozilla.org/r/153670/

> e instanceof TypeError
doesn't work probably the error is from a different context and so instanceof doesn't work

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 7 years ago

(In reply to Markus Hartung from comment #8)

> I will follow your suggestion, however I don't understand why
> Cu.isDeadWrapper(sb) doesn't return true because effectively the
> sandbox is dead, isn't it?

I think the sandbox is only marked as dead when Cu.nukeSandbox is
invoked.  In our case it just gets GCed, but not explicitly nuked.

	Andreas Tolfsen ❲:ato❳
	Comment 12 • 7 years ago

(In reply to Markus Hartung from comment #9)

> > e instanceof TypeError
> 
> doesn't work probably the error is from a different context and so
> instanceof doesn't work

Yes, we’ve had problems with this in the past.  See
testing/marionette/error.js’s error.isWebDriverError.  The reason
is, like you say, that the objects originate from different globals.

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8882564 [details]
Bug 1368068 Fix marionettes excution of javascript when previous sandbox is dead

https://reviewboard.mozilla.org/r/153670/#review158796

::: testing/marionette/evaluate.js:294
(Diff revision 1)
> + * Cu.isDeadWrapper does not return true for a dead sandbox that was assosciated with and
> + * extension popup. This provides a way to still test for a dead object.

This will fail linted checks.  Format it to ~75 characters.

::: testing/marionette/evaluate.js:298
(Diff revision 1)
> + *   A potentially dead object
> + * @param {String} prop
> + *   Name of a property on the object

Expected two more spaces for indentation of parameter descriptions.

::: testing/marionette/evaluate.js:299
(Diff revision 1)
> + * Cu.isDeadWrapper does not return true for a dead sandbox that was assosciated with and
> + * extension popup. This provides a way to still test for a dead object.
> + *
> + * @param {?} obj
> + *   A potentially dead object
> + * @param {String} prop

This should be lowercase because strings are primitives.

::: testing/marionette/evaluate.js:302
(Diff revision 1)
> + * @returns {boolean}
> + *   returns true if obj is dead

This should be:

> @return {boolean}
>     True if |obj| is dead, false otherwise.

::: testing/marionette/evaluate.js:308
(Diff revision 1)
> + *   returns true if obj is dead
> + */
> +evaluate.isDead = function(obj, prop) {
> +  try {
> +     obj[prop];
> +   } catch (e if e.message.includes("dead object")) {

The catch (e if foo) is a Gecko-proprietary extension and we shouldn’t rely on it.

This policy changed yesterday when we enabled eslint for testing/marionette.  I quite like the construct myself.

::: testing/marionette/evaluate.js:312
(Diff revision 1)
> +     obj[prop];
> +   } catch (e if e.message.includes("dead object")) {
> +     return true;
> +   }
> +   return false;
> +}

Missing semicolon.

::: testing/marionette/evaluate.js:449
(Diff revision 1)
>     *     A used or fresh sandbox.
>     */
>    get(name = "default", fresh = false) {
>      let sb = this.boxes_.get(name);
>      if (sb) {
> -      if (fresh || sb.window != this.window_) {
> +      if (fresh || evaluate.isDead(sb, 'window') || sb.window != this.window_) {

This line is probably also too long.  Try running the file through the linter:

> % ./mach lint testing/marionette/evaluate.js

	Markus Hartung Assignee
	Comment 15 • 7 years ago

I updated the request the linter should be ok now

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 16 • 7 years ago

Andreas will be back by next week. So you will have to wait until then for a next review.

	Pulsebot
	Comment 17 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ca185b005b
Check if sandbox' window is dead before evaluating script; r=ato

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8882564 [details]
Bug 1368068 Fix marionettes excution of javascript when previous sandbox is dead

https://reviewboard.mozilla.org/r/153670/#review160518

	Andreas Tolfsen ❲:ato❳
	Comment 19 • 7 years ago

Thanks for the patch.  I’ve changed the commit message slightly and pushed it to inbound.

	Carsten Book [:Tomcat]
	Comment 20 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b5ca185b005b