Save globals between execute_script calls in 'chrome' contexts

VERIFIED FIXED in mozilla37

Status

defect
VERIFIED FIXED
7 years ago
4 years ago

People

(Reporter: philikon, Assigned: chmanchester)

Tracking

({pi-marionette-server})

unspecified
mozilla37
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #751783 +++
We also need that feature for the upcoming Firefox Desktop Marionette tests. So its blocking our work on bug 1080766.

Here some testcases:

self.marionette.set_context('chrome')
self.marionette.execute_script("global.foobar = 3;")
self.assertEqual(self.marionette.execute_script("return global.foobar;", new_sandbox=False), 3)

self.marionette.set_context('chrome')
self.marionette.execute_script("this.foobar = 3;")
self.assertEqual(self.marionette.execute_script("return this.foobar;", new_sandbox=False), 3)

Particularly `global` is completely undefined, and assigning to `this` works in the first call, but the property is undefined in the second call.
Blocks: m21s
I can look into it this week.
Assignee: nobody → cmanchester
Try from comment 4 is pretty green.
Attachment #8544960 - Flags: review?(jgriffin)
/r/2097 - Bug 755036 - Re-use the execution sandbox in marionette's executeScript and executeAsyncScript in chrome scope when requested.

Pull down this commit:

hg pull review -r 8f1abb3f6573d034b3e32c86fa79220adca983fa
Chris, did you miss to add new tests for chrome? I only see the removal of two blank tests in your patch, but none which actually test the globals in chrome scope.
(In reply to Henrik Skupin (:whimboo) from comment #8)
> Chris, did you miss to add new tests for chrome? I only see the removal of
> two blank tests in your patch, but none which actually test the globals in
> chrome scope.

The chrome test classes inherit from the content ones -- taking out those stubs makes them run in chrome.
Ah, cool then! Thanks a lot for doing that so quickly!
Status: NEW → ASSIGNED
Attachment #8544960 - Flags: review?(jgriffin) → review+
https://reviewboard.mozilla.org/r/2095/#review1323

This looks good!  I think that this may fail if the user switches windows and then executes some script with new_sandbox=False, since that would cause her to be using a sandbox attached to the wrong window.  To handle that possibility, we may want to compare the sandbox's window against the current window, and throw an error if they're not the same.

In content, we handle similar situations by setting sandbox = null in switchToFrame, and we could do that here as well, but in switchToWindow.
https://reviewboard.mozilla.org/r/2095/#review1325

Aha, I'll clobber the sandbox when switching windows for consistency. Thanks!
Comment on attachment 8545402 [details] [diff] [review]
Re-use the execution sandbox in marionette's executeScript and executeAsyncScript in chrome scope when requested.

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

r=jgriffin
Attachment #8545402 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Assignee

Updated

4 years ago
Attachment #8544960 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/470bc082ec71
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
I can verify this works great now with Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0 ID:20150115030228 CSet: c1f6345f2803
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.