Closed
Bug 1353074
Opened 7 years ago
Closed 7 years ago
unloadHandler is not protected from accidental introspection in content
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
mozilla55
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(8 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
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.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Attachment #8854098 -
Flags: review?(mjzffr) → review+
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
Attachment #8854099 -
Flags: review?(mjzffr) → review+
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?
Attachment #8854100 -
Flags: review?(mjzffr) → review+
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
Attachment #8854101 -
Flags: review?(mjzffr) → review+
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
Attachment #8854102 -
Flags: review?(mjzffr) → review+
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
Attachment #8854103 -
Flags: review?(mjzffr) → review+
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?
Attachment #8854104 -
Flags: review?(mjzffr) → review-
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
Attachment #8854105 -
Flags: review?(mjzffr) → review+
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.
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.
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8854099 -
Attachment is obsolete: true
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
Attachment #8854104 -
Flags: review?(mjzffr) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Assignee | ||
Comment 37•7 years ago
|
||
Rebased and triggered new try run, to see if that helps. I couldn’t identify the crash.
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
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
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.
Flags: needinfo?(ato)
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?
Flags: needinfo?(yzenevich)
Flags: needinfo?(tbsaunde+mozbugs)
Flags: needinfo?(ato)
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?
Flags: needinfo?(tbsaunde+mozbugs)
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!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
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.
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; > });
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Updated•7 years ago
|
Attachment #8859199 -
Flags: review?(hskupin) → review?(mjzffr)
Comment hidden (mozreview-request) |
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
Attachment #8859199 -
Flags: review?(mjzffr) → review+
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
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 92•7 years ago
|
||
OK, change mentioned in comment #75 seems to have made the try run green.
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
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 95•7 years ago
|
||
\o/
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(yzenevich)
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.
Assignee | ||
Comment 97•7 years ago
|
||
Sheriffs: Please uplift to beta as test-only.
Whiteboard: [checkin-needed-beta]
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
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•