+++ 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.
David mentioned on IRC that an additional problem here could be that we also do not re-use the former sandbox even with new_sandbox=False. This seems to only work on content scope. Here some links he pasted: http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#879 http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#889 http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#922-930 http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#493-500
I can look into it this week.
Assignee: nobody → cmanchester
Try from comment 4 is pretty green.
/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!
Inbound is closed. Here's a patch with the fix suggested by jgriffin in comment 11.
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+
Attachment #8544960 - Attachment is obsolete: true
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.