executeScript fails if previously called in popup context

RESOLVED FIXED in Firefox 56

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: markus.hartung, Assigned: markus.hartung)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

2 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
(Assignee)

Comment 1

2 years ago
I tested this on 53, 54 and 55
(Assignee)

Comment 2

2 years ago
when previous sandbox is no longer available
(Assignee)

Comment 3

2 years ago
when previous sandbox is no longer available
(Assignee)

Updated

2 years ago
Attachment #8871795 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Assignee: nobody → markus.hartung
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
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.
Attachment #8871798 - Flags: feedback-
OS: Unspecified → All
Hardware: Unspecified → All
Version: 53 Branch → Trunk
(Assignee)

Comment 6

2 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?
(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.
(Assignee)

Comment 8

2 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?
(Assignee)

Comment 9

2 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
Comment hidden (mozreview-request)
(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.
(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.

Comment 13

2 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
Attachment #8882564 - Flags: review?(ato) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
I updated the request the linter should be ok now
Andreas will be back by next week. So you will have to wait until then for a next review.

Comment 17

2 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

Comment 18

2 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
Attachment #8882564 - Flags: review?(ato) → review+
Thanks for the patch.  I’ve changed the commit message slightly and pushed it to inbound.

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b5ca185b005b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.