Titus Fortner Reporter
	Description • 9 years ago

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.80 Safari/537.36

Steps to reproduce:

From one window click a link that opens a second browser window.
In the second window that has a link such as:
<a id="close" href="#" onclick="window.close()">here</a> 
Click Link


Actual results:

Nothing is returned and the execution times out.


Expected results:

click should return success with data null (http://www.w3.org/TR/webdriver/#element-click) and then additional commands should be able to be executed.

	Martijn Wargers (dead)
	Comment 1 • 9 years ago

I noticed this in Gaia UI tests as well, see bug 1164078. The workaround is to generate the click from a window that doesn't get closed.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 2 • 9 years ago

Interesting. We don't have problems with this when closing chrome windows in our Firefox UI Tests. We simply switch the chrome window once the target window has been closed, and the test continuous.

	Martijn Wargers (dead)
	Comment 3 • 9 years ago

Because chrome windows are all in the same context, I assume.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 4 • 9 years ago

So about which kind of window do we talk here? Chrome windows or tabs?

	Titus Fortner Reporter
	Comment 5 • 8 years ago

This is still an issue FF45, Wires 0.6.2

https://github.com/watir/watirspec/blob/master/browser_spec.rb#L17

html link: <a id="open" href="#" onclick="window.open("closeable.html")">here</a>

-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/url
   >>> http://127.0.0.1:4444/session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/url | {"url":"file:///Users/tfortner/git/watir_temp/watir-webdriver/spec/watirspec/html/window_switching.html"}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"105"}
<- {}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/element
   >>> http://127.0.0.1:4444/session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/element | {"using":"css selector","value":"#open"}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"40"}
<- {"value":{"element-6066-11e4-a52e-4f735466cecf":"d7ec8c20-f360-c14b-be5e-895951f5b7cf"}}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/element/d7ec8c20-f360-c14b-be5e-895951f5b7cf/click
<- {}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/title
<- {"value":"window switching"}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window
<- {"value":"7"}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window/handles
<- {"value":["7","13"]}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window
   >>> http://127.0.0.1:4444/session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window | {"handle":"13"}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"15"}
<- {}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/title
<- {"value":"closeable window"}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window/handles
<- {"value":["7","13"]}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window
   >>> http://127.0.0.1:4444/session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window | {"handle":"7"}
<- {}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window
   >>> http://127.0.0.1:4444/session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window | {"handle":"13"}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"15"}
<- {}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window
<- {"value":"13"}
-> GET session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/window/handles
<- {"value":["7","13"]}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/element
   >>> http://127.0.0.1:4444/session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/element | {"using":"css selector","value":"#close"}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"41"}
<- {"value":{"element-6066-11e4-a52e-4f735466cecf":"46039bb2-da93-aa4d-9655-0439b481c719"}}
-> POST session/34c00cb8-a50d-8a47-a7a3-02a054ba837e/element/46039bb2-da93-aa4d-9655-0439b481c719/click

Net::ReadTimeout' at /Users/tfortner/.rvm/rubies/ruby-2.2.3/lib/ruby/2.2.0/net/protocol.rb:158 - Net::ReadTimeout

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 8 years ago

Could you please provide the full Gecko log?

If you are not using a debug build (where verbose logging is enabled by default), please set the marionette.logging preference to "trace" before running the steps to reproduce again.

	Titus Fortner Reporter
	Comment 7 • 8 years ago

Where do I get a debug build?
marionette.logging looks to be a user set boolean that is already set to true.

	Titus Fortner Reporter
	Comment 8 • 8 years ago

Oh right, like developer version. Nothing recorded after this.

-> GET session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/window
1457568276582	Marionette	TRACE	conn0 -> [0,25,"getWindowHandle",{}]
1457568276582	Marionette	TRACE	conn0 <- [1,25,null,{"value":"13"}]
<- {"value":"13"}
-> GET session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/window/handles
1457568276584	Marionette	TRACE	conn0 -> [0,26,"getWindowHandles",{}]

1457568276585	Marionette	TRACE	conn0 <- [1,26,null,["7","13"]]Exception `IO::EAGAINWaitReadable' at /Users/tfortner/.rvm/rubies/ruby-2.2.3/lib/ruby/2.2.0/net/protocol.rb:153 - Resource temporarily unavailable - read would block
<- {"value":["7","13"]}
-> POST session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/frame
   >>> http://127.0.0.1:4444/session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/frame | {"id":null}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"11"}
1457568276588	Marionette	TRACE	conn0 -> [0,27,"switchToFrame",{}]
1457568276642	Marionette	DEBUG	loaded listener.js
1457568276707	Marionette	TRACE	conn0 <- [1,27,null,{}]
<- {}
-> POST session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/element
   >>> http://127.0.0.1:4444/session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/element | {"using":"css selector","value":"#close"}
     > {"Accept"=>"application/json", "Content-Type"=>"application/json; charset=utf-8", "Content-Length"=>"41"}
1457568276712	Marionette	TRACE	conn0 -> [0,28,"findElement",{"using":"css selector","value":"#close"}]
1457568276717	Marionette	TRACE	conn0 <- [1,28,null,{"value":{"ELEMENT":"5cf96a7b-88ba-9447-bf38-629e88dbb8e9","element-6066-11e4-a52e-4f735466cecf":"5cf96a7b-88ba-9447-bf38-629e88dbb8e9"}}]
<- {"value":{"element-6066-11e4-a52e-4f735466cecf":"5cf96a7b-88ba-9447-bf38-629e88dbb8e9"}}
-> GET session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/element/5cf96a7b-88ba-9447-bf38-629e88dbb8e9/name
1457568276720	Marionette	TRACE	conn0 -> [0,29,"getElementTagName",{"id":"5cf96a7b-88ba-9447-bf38-629e88dbb8e9"}]
1457568276722	Marionette	TRACE	conn0 <- [1,29,null,{"value":"a"}]
<- {"value":"a"}
-> GET session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/element/5cf96a7b-88ba-9447-bf38-629e88dbb8e9/enabled
1457568276723	Marionette	TRACE	conn0 -> [0,30,"isElementEnabled",{"id":"5cf96a7b-88ba-9447-bf38-629e88dbb8e9"}]
1457568276728	Marionette	TRACE	conn0 <- [1,30,null,{"value":true}]
<- {"value":true}
-> POST session/fac1d84b-f8dc-0747-86ad-ecaecd0f561a/element/5cf96a7b-88ba-9447-bf38-629e88dbb8e9/click
1457568276729	Marionette	TRACE	conn0 -> [0,31,"clickElement",{"id":"5cf96a7b-88ba-9447-bf38-629e88dbb8e9"}]

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

I will have a look at this bug and its current state. If this is still happening we will need an update to our unit tests too. Best fit here is test_window_management. Specifically the test method test_should_load_and_close_a_window() which would have to be changed to use window.close().

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

The hang here is caused by the promise as created by the proxy which handles the communication with frame script. In this case it is for the send() method of the AsyncMessageChannel class:

https://dxr.mozilla.org/mozilla-central/rev/7011ed1427de2b6f075c46cc6f4618d3e9fcd2a4/testing/marionette/proxy.js#100-140

Usually when sending the command to the frame script we can expect a reply, but given that the click causes the window to close, the frame script will be also killed. So there is never a reply coming back and the promise never resolves. The only escape of the promise we currently have is for modal dialogs. Whenever such a dialog appears we resolve it.

To test this I added some code to modal.js which also let us observe a possible window closing notification which in this case will be "outer-window-destroyed". With this in place we indeed catch the closing of the window and can successfully resolve the promise so that the hang is gone.

There are some side-effects which also need to be fixed given that following code in browser.js will still assume that the window is alive, and we get some JS errors. 

My idea for getting this fixed is to not resolve the promise in such a case but reject it with eg. NoSuchWindowError. It should allow us to update the browsing context appropriately to make follow-up code not failing. I will have a look into this tomorrow.

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

Before continuing on this bug I would like to fix bug 1322383 first.

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

I made lots of changes for clickElement on bug 1335778. We have to wait for those code changes to land on mozilla-central, before I could have a look here.

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

This is actually not only an issue when a click closes a window, but also when the current tab gets closed. While it looks like to be easier to check for a closed window, it's harder for tabs. Reason is that in those cases the message manager cannot be used anymore to send a reply from the frame script to the parent process.

For a former bug I already used an observer for "outer-window-destroyed" inside of listener.js. While this worked fine for this purpose, it doesn't work with proxy.js because this file is included in both the parent or content process. So when this observer is added for the parent process, it doesn't get notified if a tab closes, but only when a window closes. 

After a bit more searching I found the "message-manager-disconnect" observer notification. It looks like this is exactly what we want here. Being informed when the message manager gets disconnected because the frame script has been unloaded. In those cases we can simply finish the currently running command.

The notification has the message manager which disconnects via the subject argument. Now I'm trying to figure out a way how to check that this is the one we are currently operating on. Only then we should finish the command.

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

As it has been turned out while working on this issue the "message-manager-disconnect" is not the notification we want here. Reason is that this is also thrown when a framescript gets reloaded eg. a remoteness change. And there is no chance to figure out the underlying reason for the disconnect.

I would go with "outer-window-destroyed" for the moment so we can fix the closing window issue as it has been initially reported. For the closing tab problem I will file a new bug, which most likely depends on a fix for better message handling.

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

Sorry for the forth and back, but I actually found a way to make this working also for closing a tab. A work in progress patch will be up soon, after I cleaned-up the code.

	Andreas Tolfsen ❲:ato❳
	Comment 17 • 7 years ago
Out-Of-Date

(In reply to Henrik Skupin (:whimboo) from comment #14)
> This is actually not only an issue when a click closes a window, but also
> when the current tab gets closed. While it looks like to be easier to check
> for a closed window, it's harder for tabs. Reason is that in those cases the
> message manager cannot be used anymore to send a reply from the frame script
> to the parent process.

It seems pointless to call down to the content frame script when the browser has disappeared and the message manager is dead.  There ought to be some way of checking that the this.curBrowser is still active, and if it isn’t, return the ‘no such window’ error.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 18 • 7 years ago
Out-Of-Date

The solution I currently have is all in proxy.js. Nothing lives in listener.js. I check this by using the two events "TabClose" and "unload". 

(In reply to Andreas Tolfsen ‹:ato› from comment #17)
> has disappeared and the message manager is dead.  There ought to be some way
> of checking that the this.curBrowser is still active, and if it isn’t,
> return the ‘no such window’ error.

Do you really think we should return a `No such window` error when eg. the click command closes the tab/window? Or should the next command fail? From the spec this is not clear.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 19 • 7 years ago
Out-Of-Date

We talked about this question in our meeting, and as it turned out it was a misunderstanding. So we should clearly return normally, and the next command will fail with a `not such a window` error.

	Andreas Tolfsen ❲:ato❳
	Comment 20 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #18)
> The solution I currently have is all in proxy.js. Nothing lives in
> listener.js. I check this by using the two events "TabClose" and "unload". 

That is the same solution I’m using in my window tracking patch.

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

(In reply to Andreas Tolfsen ‹:ato› from comment #20)
> That is the same solution I’m using in my window tracking patch.

So depending on your upcoming changes my solution for proxy.js might be temporary, but fixes a P1 webdriver issue. The new two testcases are a good addition so we can ensure your changes don't regress this specific behavior.

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #25)
> (In reply to Andreas Tolfsen ‹:ato› from comment #20)
> > That is the same solution I’m using in my window tracking patch.
> 
> So depending on your upcoming changes my solution for proxy.js might be
> temporary, but fixes a P1 webdriver issue. The new two testcases are a good
> addition so we can ensure your changes don't regress this specific behavior.

I still think you need to keep this unless we install Marionette-specific notifications for when a browser we track disappears.  That might actually not be such a bad idea…

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

Might be a good idea, yes.

So I checked the try build which I pushed yesterday and it fails in two cases:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4d5a8fda2d2&selectedJob=101706545

1) Failure for Android because: "this.browser.tab.addEventListener is not a function"
2) TestCrash.test_crash_content_process fails again

I will look if I can implement what Andreas suggested, because otherwise too much browser specific code will end-up in proxy.js, which is not what we want.

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

Ted, with the changes on this bug I'm hitting a case for our Marionette crash tests which look similar to the Windows 8 test job failures we have seen a while ago. Those were races and we both haven't had an idea what's causing those. But maybe we have now...

So what I did in this patch is that I added a `unload` event handler for the chrome window. In such a case we have to early return from a command in Marionette which gets executed inside the content process. All this is fine, but our favorite crash test is starting to perma fail now:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=676f5eae2cde

The reason is the following: The execute script call will crash the content process. This is recognized by the crash handler, and given that "MOZ_CRASHREPORTER_SHUTDOWN" is set, Firefox will be closed. The first action therefore is to close each chrome window, which means our listener command as currently running returns early now. And the command does no longer notice the IOError and shutdown of Firefox. Instead it will be the next executed command. I'm not that happy about that because it can give false assumptions, and at worst the next test is marked as failing, even it didn't cause the problem.

Ted, which options do you see? Is a normal shutdown of Firefox appropriate in such a condition? I ask because there is no way to determine that something went wrong, because the exit code is also 0. :/

	(not currently active) Ted Mielczarek
	Comment 35 • 7 years ago

> Ted, which options do you see? Is a normal shutdown of Firefox appropriate in such a condition? I ask because there is no way to determine that something went wrong, because the exit code is also 0. :/

It would certainly be nicer if Firefox exited with an error code when `MOZ_CRASHREPORTER_SHUTDOWN` triggers a shutdown. I don't know if we have the machinery to do that right now, but I don't think it'd be terribly hard to support.

Your changes to the crash test don't seem bad--as long as we're verifying that a minidump was generated I don't think it matters much whether the actual Marionette command that triggered the crash triggers an exception.

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

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> Your changes to the crash test don't seem bad--as long as we're verifying
> that a minidump was generated I don't think it matters much whether the
> actual Marionette command that triggered the crash triggers an exception.

Andreas and David, would you both be ok with that? If not I will have to find a way how to circumvent an early return of the command in case of a content crash.

	David Burns :automatedtester
	Comment 37 • 7 years ago

from my point of view, do what you think is the right thing here. This seems fine to me on the face of it.

	Andreas Tolfsen ❲:ato❳
	Comment 42 • 7 years ago

I don’t think I have the full context of what is going on here, but…

(In reply to Henrik Skupin (:whimboo) from comment #36)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> > Your changes to the crash test don't seem bad--as long as we're verifying
> > that a minidump was generated I don't think it matters much whether the
> > actual Marionette command that triggered the crash triggers an exception.
> 
> Andreas and David, would you both be ok with that? If not I will have to
> find a way how to circumvent an early return of the command in case of a
> content crash.

If the browser crashes, that is a fatal event and nothing we can recover from
in Marionette.  At that point, I agree with ted that it shouldn’t matter how
we handle the command.

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

I rebased the code to latest mozilla-central, and triggered another try build to see how the changes to the content crash test are behaving.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #35)
> It would certainly be nicer if Firefox exited with an error code when
> `MOZ_CRASHREPORTER_SHUTDOWN` triggers a shutdown. I don't know if we have
> the machinery to do that right now, but I don't think it'd be terribly hard
> to support.

I filed bug 1370520 for this. Lets see what can be done, if someone has an idea.

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

The latest try build still shows failures:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b605b51a2f8

Reason is most likely that we still have a race here, and that in some cases we might still hit the IOError for the command which triggered the crash, but in other cases it's the next command. Looks like that I have to move two commands under the `assertRaises` block.

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

So the noticed crashes are a new regression on mozilla-central, and most likely related to HTTP throttling and the crash reporter initiated shutdown of Firefox. I filed bug 1371207. But until this is fixed we are on hold here. I don't want to land my patch with this particular test disabled.

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

With the help of a one-click loaner I was finally able to reproduce this crash permanently, and to solve it. The underlying issue here is just timing, and which makes it hard to correctly detect a content crash by analyzing the exception message. It would have to be caught at the right time, which is kinda unlikely given that machines are also slower.

So instead of checking the IOError message we have to check the exit code of Firefox. Only in case of content crashes it will be 0 for now. It might change in the future, but so far I think this solution is fine.

To further stabilize the test the mozcrash mock is present through the whole lifetime of a test, and not only for the crashing command. This will ensure that we can also fetch minidump files which are getting created with a delay.

A new try build has been submitted which hopefully is green now.

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

Hm, there is one remaining Error shown when running the tests:

> [task 2017-06-22T14:12:21.742592Z] 14:12:21    ERROR -  Exception socket.error: error(107, 'Transport endpoint is not connected') in <bound method TcpTransport.__del__ of <marionette_driver.transport.TcpTransport object at 0x7fb8eaa3a890>> ignored

Sadly there is no traceback available so I could check where this happens. But as it looks like we are trying to operate on self.socket which is not open anymore in `TcpTranspart.close()`:

    def close(self):
        """Close the socket."""
        if self.sock:
            try:
                self.sock.shutdown(socket.SHUT_RDWR)
            except IOError as exc:
                # Errno 57 is "socket not connected", which we don't care about here.
                if exc.errno != 57:
                    raise

Maybe of having this except clause we should check first if the socket is already closed. I can replicate a similar issue with the following small test:

    def test(self):
        self.marionette.client.close()
        self.marionette.client.close()

This results in:

> Exception socket.error: error(9, 'Bad file descriptor') in <bound method TcpTransport.__del__ of <marionette_driver.transport.TcpTransport object at 0x110da0350>> ignored

	Andreas Tolfsen ❲:ato❳
	Comment 72 • 7 years ago
mozreview-review

Comment on attachment 8870981 [details]
Bug 1223277 - Refactor clicks.html and depending unit tests.

https://reviewboard.mozilla.org/r/142546/#review157180

::: testing/marionette/harness/marionette_harness/tests/unit/test_click.py:13
(Diff revision 7)
>  
> -from marionette_harness import MarionetteTestCase, run_if_e10s, skip_if_mobile
> +from marionette_harness import (
> +    MarionetteTestCase,
> +    run_if_e10s,
> +    skip_if_mobile,
> +    WindowManagerMixin,

Unused.

	Andreas Tolfsen ❲:ato❳
	Comment 73 • 7 years ago
mozreview-review

Comment on attachment 8870982 [details]
Bug 1223277 - Log outer window id of the content window when framescript is registered.

https://reviewboard.mozilla.org/r/142548/#review157184

	Andreas Tolfsen ❲:ato❳
	Comment 74 • 7 years ago
mozreview-review

Comment on attachment 8870983 [details]
Bug 1223277 - Reorder global JS module imports in listener.js.

https://reviewboard.mozilla.org/r/142550/#review157188

	Andreas Tolfsen ❲:ato❳
	Comment 75 • 7 years ago
mozreview-review

Comment on attachment 8879574 [details]
Bug 1223277 - Improve Marionette unit tests for delayed crashes.

https://reviewboard.mozilla.org/r/150894/#review157190

	Andreas Tolfsen ❲:ato❳
	Comment 76 • 7 years ago
mozreview-review

Comment on attachment 8880731 [details]
Bug 1223277 - TcpTransport.close() should't care about errno 107.

https://reviewboard.mozilla.org/r/152100/#review157192

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 77 • 7 years ago
mozreview-review-reply

Comment on attachment 8870981 [details]
Bug 1223277 - Refactor clicks.html and depending unit tests.

https://reviewboard.mozilla.org/r/142546/#review157180

> Unused.

Ups. A left-over from debugging. It will be removed in the next update.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 84 • 7 years ago
mozreview-review-reply

Comment on attachment 8870981 [details]
Bug 1223277 - Refactor clicks.html and depending unit tests.

https://reviewboard.mozilla.org/r/142546/#review157180

> Ups. A left-over from debugging. It will be removed in the next update.

Actually it is necessary for the last commit in this patch series. So I will add it in this commit.

	Andreas Tolfsen ❲:ato❳
	Comment 85 • 7 years ago
mozreview-review

Comment on attachment 8870984 [details]
Bug 1223277 - Return immediately when click command closes tab or window.

https://reviewboard.mozilla.org/r/142552/#review157194

This approach is very similar to the one I’m taking with the window
tracking patch.  I see a future need for Marionette-specific system
notifications when a <xul:browser>/ChromeWindow appears and disappears
since it’s a bit unwieldy to have to register events on both and
having to keep a reference to the browser.Context object.

This will be a good place to begin experimenting hooking the window
tracking module up to a concrete service.

::: commit-message-5fcaa:1
(Diff revision 9)
> +Bug 1223277 - Click command has to return immediately if the current tab or window gets closed.

Not a big deal, but this is very long.  I would suggest:

> Return immediately when click closes tab or window

::: testing/marionette/proxy.js:287
(Diff revision 9)
> -  }
> -
>    addListener_(path, callback) {
>      let autoRemover = msg => {
>        this.removeListener_(path);
> -      modal.removeHandler(this.dialogueObserver_);
> +      this.removeHandlers()

Missing a semicolon.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 86 • 7 years ago
mozreview-review-reply

Comment on attachment 8870984 [details]
Bug 1223277 - Return immediately when click command closes tab or window.

https://reviewboard.mozilla.org/r/142552/#review157194

Right, and I feel more confident in doing such a refactoring as more tests we actually get. I hope it's not that far away anymore. 

Thanks for the reviews!

	Pulsebot
	Comment 88 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/afbaa1138de3
Refactor clicks.html and depending unit tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/547744157e2a
Log outer window id of the content window when framescript is registered. r=ato
https://hg.mozilla.org/integration/autoland/rev/b9f95fc81d23
Reorder global JS module imports in listener.js. r=ato
https://hg.mozilla.org/integration/autoland/rev/e6921a7229a3
Improve Marionette unit tests for delayed crashes. r=ato
https://hg.mozilla.org/integration/autoland/rev/aacd8d27b192
TcpTransport.close() should't care about errno 107. r=ato
https://hg.mozilla.org/integration/autoland/rev/85c86dc86122
Return immediately when click command closes tab or window. r=ato

	Pulsebot
	Comment 89 • 7 years ago

Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a2c5991d1fe9
Backed out 6 changesets for causing Marionette test regressions.

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

We had to backout the patch due to failures I noticed on try:

https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=mn&bugfiler&tochange=a2c5991d1fe9&fromchange=85c86dc86122

At least the following are visible:

1) If a crash test fails we do not correctly restore the mozcrash module, but the mock stays active. Due to that for all following tests we do no longer correctly check for crashes.

2) We got a new assertion for other crash tests like: AssertionError: "Process crashed" does not match "Process has been unexpectedly closed (Exit code: 1) (Reason: No data received over socket)". This is most likely due to the delayed crash checks, and might need a similar fix like I did for the content crash test. This is only visible for ASAN builds.

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

(In reply to Henrik Skupin (:whimboo) from comment #90)
> We had to backout the patch due to failures I noticed on try:
> 
> https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-
> searchStr=mn&bugfiler&tochange=a2c5991d1fe9&fromchange=85c86dc86122
> 
> 2) We got a new assertion for other crash tests like: AssertionError:
> "Process crashed" does not match "Process has been unexpectedly closed (Exit
> code: 1) (Reason: No data received over socket)". This is most likely due to
> the delayed crash checks, and might need a similar fix like I did for the
> content crash test. This is only visible for ASAN builds.

So this problem is actually only happening for ASAN builds and as it looks like it is because the minidump files are getting created with a delay of a couple of seconds. It means when Firefox got closed after a crash, Marionette checks for the minidump files immediately but doesn't recognize any of those and as such reports a count of 0. I'm not sure how to get this fixed and as discussed with David and Andreas we will turn off our Marionette unit tests for ASAN builds.

Ted, if you have an idea please let me know. We could think about improvements later. Also CC'ing Joel to inform him about the disabling step.

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

Last try build is all green. So I think we are good in proceeding with the proposal:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad932fabe9013d26e4d7ecc91fd9b25b1826247c

	Andreas Tolfsen ❲:ato❳
	Comment 96 • 7 years ago
mozreview-review

Comment on attachment 8881487 [details]
Bug 1223277 - Disable Marionette unit tests for ASAN builds.

https://reviewboard.mozilla.org/r/152626/#review157804

	Pulsebot
	Comment 97 • 7 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2bb35f565c0d
Refactor clicks.html and depending unit tests. r=ato
https://hg.mozilla.org/integration/autoland/rev/c21a8fdf6313
Log outer window id of the content window when framescript is registered. r=ato
https://hg.mozilla.org/integration/autoland/rev/35fe9fc51464
Reorder global JS module imports in listener.js. r=ato
https://hg.mozilla.org/integration/autoland/rev/764ff615e2ab
Improve Marionette unit tests for delayed crashes. r=ato
https://hg.mozilla.org/integration/autoland/rev/b70784240220
Disable Marionette unit tests for ASAN builds. r=ato
https://hg.mozilla.org/integration/autoland/rev/961f3a36450e
TcpTransport.close() should't care about errno 107. r=ato
https://hg.mozilla.org/integration/autoland/rev/fd18fb0e7c40
Return immediately when click command closes tab or window. r=ato

	Carsten Book [:Tomcat]
	Comment 98 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/2bb35f565c0d
https://hg.mozilla.org/mozilla-central/rev/c21a8fdf6313
https://hg.mozilla.org/mozilla-central/rev/35fe9fc51464
https://hg.mozilla.org/mozilla-central/rev/764ff615e2ab
https://hg.mozilla.org/mozilla-central/rev/b70784240220
https://hg.mozilla.org/mozilla-central/rev/961f3a36450e
https://hg.mozilla.org/mozilla-central/rev/fd18fb0e7c40

	(not currently active) Ted Mielczarek
	Comment 100 • 7 years ago

Sorry for not getting back to this, but ASAN builds disable the crashreporter, so tests for crash reporting should not be run on them.

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

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #100)
> Sorry for not getting back to this, but ASAN builds disable the
> crashreporter, so tests for crash reporting should not be run on them.

Thanks Ted. I filed bug 1410355 to get the remaining unit tests re-enabled for ASAN builds.