706744 - Closing Firefox opened by cfx run does not terminate the cfx run command.

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

markh brought this up in IRC.

I pulled recently from github master and created an addon using cfx init. I then ran cfx run (twice, to pick up the ID), and it opened a Firefox window.

I then closed the window, expecting cfx run to print "total time:" and "Program terminated successfully.", but it just hung there after "info: The add-on is running."

Using git bisect, I narrowed it down to [03fd1498b4a39beb0fef63c3ce7feadd0ef19333] Bug 660629 - Simplify XPI layout and manifest by specifying only module path and registering only one resource domain.


[22:18]	KWierso|win8	markh: Bisecting: 0 revisions left to test after this (roughly 0 steps)
[22:18]	KWierso|win8	[03fd1498b4a39beb0fef63c3ce7feadd0ef19333] Simplify XPI layout and manifest by specifying only module path and registering only one resource domain.
[22:18]	KWierso|win8	looks at ochameau...
[22:21]	markh	hrm - that touches the line in system.js that isn't working...
[22:21]	markh	and reverting that line fixes it
[22:22]	KWierso|win8	markh: I'm filing a bug on it
[22:22]	markh	packages/api-utils/lib/system.js
[22:22]	markh	- file.open(options.resultFile, 'w').write(code ? 'FAIL' : 'OK');
[22:22]	markh	+ file.open(options.resultFile, 'w').writeAsync(code ? 'FAIL' : 'OK');
[22:22]	markh	and it works again

Comment 1

[Mark Hammond [:markh] [:mhammond]]

I think the problem is that writeAsync closes the stream whereas write doesn't - hence the output is stuck in a buffer.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Close stream and add unit test

You were right, I didn't knew we had to close the stream. There is a difference between asynchronous and synchronous API.
This is not the first time we have an issue where cfx doesn't quit when firefox does, so I've added simple unit test to ensure that!

Comment 3

[Alexandre Poirot [:ochameau]]

This regression was due to bug 698550. (bug 660629 have not replaced writeAsync to write)

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 578263 [details] [diff] [review]
Close stream and add unit test

Looks good to me and after the patch I can't reproduce the initial problem and all tests pass.

Comment 5

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/cb47c7f5d8416a11dd46c01681f9859671f307a3

Comment 6

[Alexandre Poirot [:ochameau]]

I had to revert this change as it doesn't pass on tinderbox:
https://github.com/mozilla/addon-sdk/commit/4d1f95ab059f6ece1476d5e7da1f86a3234ddf8d

http://tinderbox.mozilla.org/showlog.cgi?log=Jetpack/1323093837.1323094324.15582.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Jetpack/1323082440.1323082893.9743.gz

There is multiple issues, so that I can't hotfix it easily. I'll require many modification in order to make it pass on python 2.5 :(
 - SystemExit is empty on 2.5: `code` and `args` attributes are empty
 - the sub cfx command we are running is using firefox 3.6 binary (on my system). I don't know why it is not able to find other binaries.
 - assertIn doesn't exists in python 2.5

Comment 7

[Alexandre Poirot [:ochameau]]

Attachment: Pull request 286

Here is a new patch on top of previous with fixes specific to python 2.5.
I figured out what was wrong with firefox 3.6, my windows registry was pointing to Firefox 3.6 folder for an unknown reason ... and for some other reason, firefox path is using registry only with python 2.5?!

Comment 8

[Alexandre Poirot [:ochameau]]

Followup landings:
- missing file from PR 286:
https://github.com/mozilla/addon-sdk/commit/b140e9edd16f653182916e3a1d66eccbb484192c
- linux hotfix
https://github.com/mozilla/addon-sdk/commit/06cb645250e2e627c3df5ef8d4580948e35e8956
- OSX bug has been fixed through bug 708163 and :
https://github.com/mozilla/addon-sdk/commit/09f61e37c893a5e914b499f499089667d37a37c2

Everything is now green, considering this bug as fixed!

Comment 9

[[github robot]]

Commits pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d19efe4cfc8253ec03715d964e1e0b27acf37d3d
Bug 706744: Closing Firefox opened by cfx run does not terminate the cfx
run command.

https://github.com/mozilla/addon-sdk/commit/fca0eee28efe28d33179d502d6b27ce62fa71b3c
Merge pull request #294 from ochameau/cfx-quit-stab

Bug 706744 - Closing Firefox opened by cfx run does not terminate the cfx run command. (backport of multiple changes on master branch)

Comment 10

[[github robot]]

Commits pushed to https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/d19efe4cfc8253ec03715d964e1e0b27acf37d3d
Bug 706744: Closing Firefox opened by cfx run does not terminate the cfx

https://github.com/mozilla/addon-sdk/commit/fca0eee28efe28d33179d502d6b27ce62fa71b3c
Merge pull request #294 from ochameau/cfx-quit-stab

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 579160 [details]
Pull request 286

This has already landed.