Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

As is exemplified in https://github.com/mozilla/geckodriver/issues/515#issuecomment-291208304, Marionette does not protected the unloadHandler in testing/marionette/evaluate.js from accidental modification or introspection when window.removeEventHandler is overridden.

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

Test case to reproduce:

>     def test_override_remove_event_listener(self):
>         self.marionette.navigate(inline("""
>             <script>
>             window.removeEventListener = (event, listener) => listener.toString();
>             </script>
>             """))
>         self.marionette.execute_script("", sandbox=None)

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8854098 [details]
Bug 1353074 - Make unload event safe for introspection from content;

https://reviewboard.mozilla.org/r/126068/#review129094

::: testing/marionette/evaluate.js:269
(Diff revision 1)
>  };
>  
>  sandbox.createSimpleTest = function (window, harness) {
>    let sb = sandbox.create(window);
>    sb = sandbox.augment(sb, harness);
>    sb[FINISH] = () => sb[COMPLETE](harness.generate_results());

This is hitting a Permission Denied error on your try push.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8854099 [details]
Bug 1353074 - Protect __webDriverComplete global from introspection;

https://reviewboard.mozilla.org/r/126070/#review129096

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8854100 [details]
Bug 1353074 - Use tuples for script arguments;

https://reviewboard.mozilla.org/r/126072/#review129098

::: commit-message-d8d3e:4
(Diff revision 1)
> +Bug 1353074 - Use tuples for script arguments; r?maja_zf
> +
> +We only allow a list type for backwards compatibility.  Tuples is what is
> +recommended because of interoperability with the Python standard library.

Nit: I can imagine future readers being puzzled by this statement because it's kind of broad/vague. Do you mean we prefer tuples as arguments because they are immutable?

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8854101 [details]
Bug 1353074 - Test arguments in all sandboxes;

https://reviewboard.mozilla.org/r/126074/#review129104

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 14 • 7 years ago
mozreview-review

Comment on attachment 8854102 [details]
Bug 1353074 - Run globals execute script tests in all sandboxes;

https://reviewboard.mozilla.org/r/126076/#review129118

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8854103 [details]
Bug 1353074 - Run Components permission test in all sandboxes;

https://reviewboard.mozilla.org/r/126078/#review129120

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8854104 [details]
Bug 1353074 - Run wrappedJSObject execute script tests in all sandboxes;

https://reviewboard.mozilla.org/r/126080/#review129122

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py:213
(Diff revision 1)
> -    def test_wrappedjsobject(self):
> +    def test_mutable_sandbox_wrappedjsobject(self):
> +        self.assert_is_defined("window.wrappedJSObject")
> +        with self.assertRaises(errors.JavascriptException):
> +            self.marionette.execute_script("window.wrappedJSObject.foo = 1", sandbox=None)
> +
> +    def test_mutable_sandbox_wrappedjsobject(self):
> +        self.assert_is_defined("window.wrappedJSObject", sandbox=None)

Two test methods with same name. It seems like one is extra/left-over

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py:242
(Diff revision 1)
>              "window.wrappedJSObject.foo = 4", sandbox="system")
>          self.assertEqual(self.marionette.execute_script(
>              "return window.wrappedJSObject.foo", sandbox="system"), 4)
>  
>      def test_system_dead_object(self):
> +        self.assert_is_defined("window.wrappedJSObject")

Why not `sandbox="system"` here?

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8854105 [details]
Bug 1353074 - Components ctor test should not throw;

https://reviewboard.mozilla.org/r/126082/#review129124

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

Comment on attachment 8854098 [details]
Bug 1353074 - Make unload event safe for introspection from content;

https://reviewboard.mozilla.org/r/126068/#review129094

> This is hitting a Permission Denied error on your try push.

Ah, so that’s the reason I didn’t fix this the last time around.  I’m going to revert the change to clone sb[COMPLETE].  I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1353696 to follow up on this later, as it’s strictly not related to what I’m trying to fix in this bug.

This happens because harness.generate_results() originates from chrome space, and sb[COMPLETE] has already been cloned into the content sandbox.  This prevents us from falling it again from chrome.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 19 • 7 years ago
mozreview-review-reply

Comment on attachment 8854100 [details]
Bug 1353074 - Use tuples for script arguments;

https://reviewboard.mozilla.org/r/126072/#review129098

> Nit: I can imagine future readers being puzzled by this statement because it's kind of broad/vague. Do you mean we prefer tuples as arguments because they are immutable?

Not exactly, but that too is a really good reason!  I mean that we prefer them because the standard library uses tuples whenever it defines arguments for functions.  In other words: varargs in Python are tuples.

I have clarified the commit message somewhat.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 20 • 7 years ago
mozreview-review-reply

Comment on attachment 8854104 [details]
Bug 1353074 - Run wrappedJSObject execute script tests in all sandboxes;

https://reviewboard.mozilla.org/r/126080/#review129122

> Two test methods with same name. It seems like one is extra/left-over

Oh, good catch (-:

> Why not `sandbox="system"` here?

Mistake by omission.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 28 • 7 years ago
mozreview-review

Comment on attachment 8854104 [details]
Bug 1353074 - Run wrappedJSObject execute script tests in all sandboxes;

https://reviewboard.mozilla.org/r/126080/#review129442

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

There are issues with Mn-e10s on Windows 7 VM debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=831492ac775f&group_state=expanded&selectedJob=88873454

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

Rebased and triggered new try run, to see if that helps.  I couldn’t identify the crash.

	Pulsebot
	Comment 38 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2670eec1ed8a
Make unload event safe for introspection from content; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/8112153e0793
Use tuples for script arguments; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/23409efe536f
Test arguments in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/8dcd190a0a59
Run globals execute script tests in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/9c1ca76fba9b
Run Components permission test in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/001f220710a2
Run wrappedJSObject execute script tests in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/0c075043bc43
Components ctor test should not throw; r=maja_zf

	Phil Ringnalda (:philor)
	Comment 39 • 7 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/7c7029cf53f5 for linux and windows debug e10s crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=89428248&repo=autoland

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

This is still failing (randomly?) on the Windows 7 VM debug Mn-e10s job.  I have no idea what is causing this.  The failure seems pretty obscure.

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

(In reply to Andreas Tolfsen ‹:ato› from comment #40)
> This is still failing (randomly?) on the Windows 7 VM debug Mn-e10s job.  I
> have no idea what is causing this.  The failure seems pretty obscure.

I had a look at the crash report and it tells EXCEPTION_BREAKPOINT. It means that the tab the test was running in got killed due to a hang. So you are facing an content hang here. It seems to also be related to accessibility.

The following assertion (sadly still with garbled frames) is visible in the gecko.log:

Assertion failure: AccessAllowed(), at c:\builds\moz2_slave\autoland-w32-d-000000000000000\build\src\obj-firefox\dist\include\mozilla/Dispatcher.h:89
#01: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0xee2659]
#02: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x1028a70]
#03: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0xf69fee]
#04: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x1023730]
#05: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0xff9115]
#06: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x2007bd9]
#07: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x1fe7f5a]
#08: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x2010529]
#09: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x1ff16ca]
#10: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x2022ed7]
#11: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x2022b33]
#12: DumpFrameArray[c:\slave\test\build\application\firefox\xul.dll +0x290328a]
#13: DumpFrameArray[c:\slave\test\build\application\firefox\xul.dll +0x28fd525]
#14: DumpFrameArray[c:\slave\test\build\application\firefox\xul.dll +0x29024ed]
#15: ???[c:\slave\test\build\application\firefox\xul.dll +0x3b7bbb]
#16: ???[c:\slave\test\build\application\firefox\xul.dll +0x3b9a03]
#17: ???[c:\slave\test\build\application\firefox\xul.dll +0x3bdba1]
#18: ???[c:\slave\test\build\application\firefox\xul.dll +0x3ba35e]
#19: ???[c:\slave\test\build\application\firefox\xul.dll +0x3bc9fa]
#20: ???[c:\slave\test\build\application\firefox\xul.dll +0x3ba473]
#21: XRE_GetBootstrap[c:\slave\test\build\application\firefox\xul.dll +0x2c9e1e6]
#22: XRE_GetBootstrap[c:\slave\test\build\application\firefox\xul.dll +0x2cab53d]
#23: XRE_GetBootstrap[c:\slave\test\build\application\firefox\xul.dll +0x2ca1dea]
#24: XRE_GetBootstrap[c:\slave\test\build\application\firefox\xul.dll +0x2ca9875]
#25: XRE_GetBootstrap[c:\slave\test\build\application\firefox\xul.dll +0x2c88f9c]
#26: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x101a4a6]
#27: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x101a64a]
#28: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x10162da]
#29: ???[c:\slave\test\build\application\firefox\xul.dll +0x428748]
#30: ???[c:\slave\test\build\application\firefox\xul.dll +0x421a68]
#31: ???[c:\slave\test\build\application\firefox\xul.dll +0x42122c]
#32: ???[c:\slave\test\build\application\firefox\xul.dll +0x42b46e]
#33: ???[c:\slave\test\build\application\firefox\xul.dll +0x42aa77]
#34: soundtouch::SoundTouch::operator=[c:\slave\test\build\application\firefox\xul.dll +0x7ebd55]
#35: soundtouch::SoundTouch::operator=[c:\slave\test\build\application\firefox\xul.dll +0x7ebe58]
#36: ???[c:\slave\test\build\application\firefox\xul.dll +0x7c4744]
#37: ???[c:\slave\test\build\application\firefox\xul.dll +0x7c46fc]
#38: ???[c:\slave\test\build\application\firefox\xul.dll +0x7c4470]
#39: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x1e06929]
#40: mozilla_dump_image[c:\slave\test\build\application\firefox\xul.dll +0x1e5a772]
#41: workerlz4_maxCompressedSize[c:\slave\test\build\application\firefox\xul.dll +0x2b67c7d]
#42: soundtouch::SoundTouch::operator=[c:\slave\test\build\application\firefox\xul.dll +0x7ebdcd]
#43: ???[c:\slave\test\build\application\firefox\xul.dll +0x7c4744]
#44: ???[c:\slave\test\build\application\firefox\xul.dll +0x7c46fc]
#45: ???[c:\slave\test\build\application\firefox\xul.dll +0x7c4470]
#46: workerlz4_maxCompressedSize[c:\slave\test\build\application\firefox\xul.dll +0x2b677f3]
#47: workerlz4_maxCompressedSize[c:\slave\test\build\application\firefox\xul.dll +0x2b708aa]
#48: ???[c:\slave\test\build\application\firefox\firefox.exe +0x1574]
#49: ???[c:\slave\test\build\application\firefox\firefox.exe +0x1330]
#50: ???[c:\slave\test\build\application\firefox\firefox.exe +0x1a69]
#51: TargetNtUnmapViewOfSection[c:\slave\test\build\application\firefox\firefox.exe +0x363d9]
#52: BaseThreadInitThunk[C:\windows\system32\kernel32.dll +0x53c45]
#53: RtlInitializeExceptionChain[C:\windows\SYSTEM32\ntdll.dll +0x637f5]
#54: RtlInitializeExceptionChain[C:\windows\SYSTEM32\ntdll.dll +0x637c8]

###!!! [Parent][MessageChannel] Error: (msgtype=0x2C008D,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x2C0080,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv


Trevor or Yuri, could this be related to the other content crashes/hangs (like bug 1347451) we see with accessibility at the moment?

	Trevor Saunders (:tbsaunde)
	Comment 42 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #41)
> (In reply to Andreas Tolfsen ‹:ato› from comment #40)
> > This is still failing (randomly?) on the Windows 7 VM debug Mn-e10s job.  I
> > have no idea what is causing this.  The failure seems pretty obscure.
> 
> I had a look at the crash report and it tells EXCEPTION_BREAKPOINT. It means
> that the tab the test was running in got killed due to a hang. So you are
> facing an content hang here. It seems to also be related to accessibility.

are you sure? it looks to me like Gecko failed an assert, leading to that stack and the breakpoint trap.  The assert seems to have something to do with message loop dispatch, but I'm not sure.

> #54: RtlInitializeExceptionChain[C:\windows\SYSTEM32\ntdll.dll +0x637c8]
> 
> ###!!! [Parent][MessageChannel] Error:
> (msgtype=0x2C008D,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel
> error: cannot send/recv

and then we hit this because the child process is gone.

> Trevor or Yuri, could this be related to the other content crashes/hangs
> (like bug 1347451) we see with accessibility at the moment?

doesn't seem like it?

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

Thank you Trevor! So I had another look and this line of code is triggering the crash:

1491540460111	Marionette	TRACE	135 -> [0,23,"executeScript",{"scriptTimeout":null,"newSandbox":true,"args":[{"element-6066-11e4-a52e-4f735466cecf":"e89078b7-55fb-4e99-8705-159417093c28","ELEMENT":"e89078b7-55fb-4e99-8705-159417093c28"}],"filename":"selection.py","script":"arguments[0].setSelectionRange(0, 0);","sandbox":"default","line":72}]

Andreas, how often does it fail? I wonder if you could change the test for debugging purposes to use different values for the sandbox - if that makes sense. But it looks like Fx is not happy here, or you may have revealed an underlying bug in Firefox even!

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

(In reply to Henrik Skupin (:whimboo) from comment #43)
> So I had another look and this line of code is triggering the crash:
> 
> 1491540460111	Marionette	TRACE	135 ->
> [0,23,"executeScript",{"scriptTimeout":null,"newSandbox":true,"args":
> [{"element-6066-11e4-a52e-4f735466cecf":"e89078b7-55fb-4e99-8705-
> 159417093c28","ELEMENT":"e89078b7-55fb-4e99-8705-159417093c28"}],"filename":
> "selection.py","script":"arguments[0].setSelectionRange(0,
> 0);","sandbox":"default","line":72}]

That’s interesting.  Thanks for looking a bit deeper at this, whimboo!

That command gets sent as a result of this client code:

http://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/selection.py#72:
>     def move_cursor_to_front(self):
>         '''Move cursor in the element to the front of the content.'''
>         if self._input_or_textarea():
>             cmd = '''arguments[0].setSelectionRange(0, 0);'''
>         else:
>             cmd = '''var sel = window.getSelection();
>                   sel.collapse(arguments[0].firstChild, 0);'''
> 
>         self.element.marionette.execute_script(cmd, script_args=[self.element])

There’s nothing obviously wrong with this code, except maybe for the fact that the script is executed in the default sandbox.  Most of the other execute script calls in this file uses the system sandbox, but that’s presumably to store information across execute calls.

Of the patches attached, the only likely suspect is https://reviewboard.mozilla.org/r/126068/diff/3#index_header: it clones the unload document handler into the sandbox and attaches it using addEventListener instead of assigning to sb.window.onload.

> Andreas, how often does it fail?

According to the try run I ran before sending this off to Autoland, not very frequently: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c371f7a760c

> I wonder if you could change the test for
> debugging purposes to use different values for the sandbox - if that makes
> sense. But it looks like Fx is not happy here, or you may have revealed an
> underlying bug in Firefox even!

Good idea.  I made the following changes:

> diff --git a/testing/marionette/client/marionette_driver/selection.py b/testing/marionette/client/marionette_driver/selection.py
> index 30e66de..b89736b 100644
> --- a/testing/marionette/client/marionette_driver/selection.py
> +++ b/testing/marionette/client/marionette_driver/selection.py
> @@ -59,7 +59,7 @@ class SelectionManager(object):
>                '''.format(offset, 'backward' if backward else 'forward')
>  
>          self.element.marionette.execute_script(
> -            cmd, script_args=[self.element], sandbox='system')
> +            cmd, script_args=(self.element,), sandbox='system')
>  
>      def move_cursor_to_front(self):
>          '''Move cursor in the element to the front of the content.'''
> @@ -69,7 +69,8 @@ class SelectionManager(object):
>              cmd = '''var sel = window.getSelection();
>                    sel.collapse(arguments[0].firstChild, 0);'''
>  
> -        self.element.marionette.execute_script(cmd, script_args=[self.element])
> +        self.element.marionette.execute_script(
> +            cmd, script_args=(self.element,), sandbox=None)
>  
>      def move_cursor_to_end(self):
>          '''Move cursor in the element to the end of the content.'''
> @@ -80,7 +81,8 @@ class SelectionManager(object):
>              cmd = '''var sel = window.getSelection();
>                    sel.collapse(arguments[0].lastChild, arguments[0].lastChild.length);'''
>  
> -        self.element.marionette.execute_script(cmd, script_args=[self.element])
> +        self.element.marionette.execute_script(
> +            cmd, script_args=(self.element,), sandbox=None)
>  
>      def selection_rect_list(self, idx):
>          '''Return the selection's DOMRectList object for the range at given idx.
> @@ -92,17 +94,15 @@ class SelectionManager(object):
>          '''
>          cmd = self.js_selection_cmd() +\
>              '''return sel.getRangeAt({}).getClientRects();'''.format(idx)
> -        return self.element.marionette.execute_script(cmd,
> -                                                      script_args=[self.element],
> -                                                      sandbox='system')
> +        return self.element.marionette.execute_script(
> +            cmd, script_args=(self.element,), sandbox='system')
>  
>      def range_count(self):
>          '''Get selection's range count'''
>          cmd = self.js_selection_cmd() +\
>              '''return sel.rangeCount;'''
> -        return self.element.marionette.execute_script(cmd,
> -                                                      script_args=[self.element],
> -                                                      sandbox='system')
> +        return self.element.marionette.execute_script(
> +            cmd, script_args=(self.element,), sandbox='system')
>  
>      def _selection_location_helper(self, location_type):
>          '''Return the start and end location of the selection in the element.
> @@ -207,7 +207,8 @@ class SelectionManager(object):
>                    sel.removeAllRanges();
>                    sel.addRange(range);'''
>  
> -        self.element.marionette.execute_script(cmd, script_args=[self.element])
> +        self.element.marionette.execute_script(
> +            cmd, script_args=(self.element,), sandbox=None)
>  
>      @property
>      def content(self):
> @@ -222,6 +223,5 @@ class SelectionManager(object):
>          '''Return the selected portion of the content in the element.'''
>          cmd = self.js_selection_cmd() +\
>              '''return sel.toString();'''
> -        return self.element.marionette.execute_script(cmd,
> -                                                      script_args=[self.element],
> -                                                      sandbox='system')
> +        return self.element.marionette.execute_script(
> +            cmd, script_args=(self.element,), sandbox='system')

The try run with the above changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aba59a19621e17b41bf7f49948ef6dc2717ba63

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

Another theory is that because the "default" sandbox is being reused, using sb.window.addEventListener could potentially result in multiple unload event listeners being added.  I’m not entirely sure this would cause a crash, but I’m not entirely sure why this would cause a crash in AccessibleCaret.cpp.

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

(In reply to Andreas Tolfsen ‹:ato› from comment #52)
> Another theory is that because the "default" sandbox is being reused, using
> sb.window.addEventListener could potentially result in multiple unload event
> listeners being added.  I’m not entirely sure this would cause a crash, but
> I’m not entirely sure why this would cause a crash in AccessibleCaret.cpp.

This was an incorrect statement from my side.  The unload event listener is being removed on :170, as can be seen from this excerpt:

http://searchfox.org/mozilla-central/source/testing/marionette/evaluate.js#170:
>   return promise.then(res => {
>     clearTimeout(scriptTimeoutID);
>     sb.window.removeEventListener("unload", unloadHandler);
>     return res;
>   });

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

Latest patch set makes use of mutable sandboxes for the Marionette selection API.  This causes the crashes to go away, but I still see various intermittents in https://treeherder.mozilla.org/#/jobs?repo=try&revision=8aba59a19621e17b41bf7f49948ef6dc2717ba63 which I’m not sure are related to the changes made here.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 64 • 7 years ago
mozreview-review

Comment on attachment 8859199 [details]
Bug 1353074 - Use mutable sandboxes for selection API;

https://reviewboard.mozilla.org/r/131244/#review133904

	Pulsebot
	Comment 65 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7af70f65c2c
Make unload event safe for introspection from content; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/764220600b06
Use tuples for script arguments; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/811a3e45bf11
Test arguments in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/28eb1c784875
Run globals execute script tests in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/c8d2b2c701a6
Run Components permission test in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/56027ec27fb8
Run wrappedJSObject execute script tests in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/a6249ce2b09e
Components ctor test should not throw; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/824c46ad23f9
Use mutable sandboxes for selection API; r=maja_zf

	Phil Ringnalda (:philor)
	Comment 66 • 7 years ago

Backed out in https://hg.mozilla.org/integration/autoland/rev/245811243903 for https://treeherder.mozilla.org/logviewer.html#?job_id=92526633&repo=autoland and https://treeherder.mozilla.org/logviewer.html#?job_id=92525711&repo=autoland

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

Pushed a change where I made the following change, restoring the original behaviour:

> -    sb.window.addEventListener("unload", unloadHandler);
> +    sb.window.onunload = unloadHandler;

I suspect it is this that’s causing the widespread instability.

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

OK, change mentioned in comment #75 seems to have made the try run green.

	Pulsebot
	Comment 93 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa0c99211baa
Make unload event safe for introspection from content; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/2e1d7f5759ca
Use tuples for script arguments; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/216ae291fec3
Test arguments in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/18714fe2dd5d
Run globals execute script tests in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/196923b18551
Run Components permission test in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/9c47b7a2f060
Run wrappedJSObject execute script tests in all sandboxes; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/b7a9e3b4ec0a
Components ctor test should not throw; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/3daa2953df6b
Use mutable sandboxes for selection API; r=maja_zf

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 94 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fa0c99211baa
https://hg.mozilla.org/mozilla-central/rev/2e1d7f5759ca
https://hg.mozilla.org/mozilla-central/rev/216ae291fec3
https://hg.mozilla.org/mozilla-central/rev/18714fe2dd5d
https://hg.mozilla.org/mozilla-central/rev/196923b18551
https://hg.mozilla.org/mozilla-central/rev/9c47b7a2f060
https://hg.mozilla.org/mozilla-central/rev/b7a9e3b4ec0a
https://hg.mozilla.org/mozilla-central/rev/3daa2953df6b

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

\o/

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

We discussed uplift of this changeset at the Marionette meeting yesterday.  Our conclusion is that it’s a good candidate, given that it’s a bug fix that impacts a large number of users using geckodriver against Angular-based websites.

We want to be more thoughtful about uplifts in general, but this is in my opinion an ideal candidate for uplift since it is (a) a bug fix and (b) touches code not changed recently.  I would estimate that the risk of this uplift would be moderate, considering the number of problems I had landing it on central.

We will let this patch mature for some time on mozilla-central (Nightly), since there is still plenty of time before the next uplift on 12 June 2017.

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

Sheriffs: Please uplift to beta as test-only.

	Ryan VanderMeulen [:RyanVM]
	Comment 98 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/0249a5098344
https://hg.mozilla.org/releases/mozilla-beta/rev/18868d8ce0ac
https://hg.mozilla.org/releases/mozilla-beta/rev/e5864fa0e413
https://hg.mozilla.org/releases/mozilla-beta/rev/75a6345f739f
https://hg.mozilla.org/releases/mozilla-beta/rev/1f8a283d4a99
https://hg.mozilla.org/releases/mozilla-beta/rev/d47a756d658a
https://hg.mozilla.org/releases/mozilla-beta/rev/6f07c769efdd
https://hg.mozilla.org/releases/mozilla-beta/rev/9bef2dfbd584