Broken phantom.exit() ?

[Nicolas Perriault]

Hi,

I've just compiled 1.7-dev from latest master, and a simple script like:

phantom.exit();
console.log('plop');

will actually print "plop" to the console AND exits.

Shouldn't the correct behavior be to just exit without processing
further statements?

Cheers,

--
Nicolas Perriault
https://nicolas.perriault.net/ — http://www.akei.com/
Skype: nperriault
Phone: +33 (0) 660 92 08 67

[Ariya Hidayat]

> phantom.exit();
> console.log('plop');
>
> will actually print "plop" to the console AND exits.
>
> Shouldn't the correct behavior be to just exit without processing
> further statements?

Isn't this always the behavior? There is long history behind it
(search the issue tracker) but the main problem is technical, esp. on
Windows.



--
Ariya Hidayat, http://ariya.ofilabs.com
http://twitter.com/ariyahidayat

[James Greene]

If I have any statements after the call to `phantom.exit`, I typically get an error/exception. (1.6.1 on Windows... did not test this scenario with my 1.7-pre local build)

[Nicolas Perriault]

On Sat, Sep 22, 2012 at 6:19 PM, Ariya Hidayat <ariya....@gmail.com> wrote:

> Isn't this always the behavior?

Nope:

$ echo "phantom.exit();console.log('plop')" > test.js
$ phantomjs --version
1.6.1
$ phantomjs test.js
$ phantomjs17 --version
1.7.0 (development)
$ phantomjs17 test.js
plop

Hint: fresh compilation of phantomjs latest master on OSX Lion 10.7.5,
gcc i686-apple-darwin11-llvm-gcc-4.2 (GCC) 4.2.1

++

[James Greene]

Hmm... I slightly retract this statement.  Seems that most lines of code after `phantom.exit()` do not cause the errors I described but I do occasionally run into that.  Perhaps it depends on the functionality being invoked...?

Anyway, I ran Nicolas's test script on PhantomJS 1.5.0, 1.6.0, and 1.6.1 for Windows and the post-exit console messages were not printed in any of those versions.

~~James

[Shawn Krisman]

phantom.exit() never prevented code from being run it simply hid many of the side effects. You can test this out by running this code on 1.6:

phantom.exit();
phantom.injectJs('hey!')

The above causes a segfault because the code made mistakes in some of its side effect hiding logic. I fixed the above issue with this pull request https://github.com/ariya/phantomjs/pull/302/. Unfortunately this pull request being perhaps a bit too ambitious got rid of a bunch of this logic and simply embraced the fact that we don't run prevent code from running after phantom.exit(). 

I've made a less ambitious change that prevents the segfault above as well as preserving the side effect hiding logic of before:

https://github.com/ariya/phantomjs/pull/322

I've been looking a lot on how to actually prevent code from running after phantom.exit and it looks really difficult. The problem is that all the code we use to run js is an all or nothing kind of thing. For instance QWebFrame::evaluateJavaScript. You can't tell it to return from the call early, you can only wait until its done evaluating. I've thought of a number of solutions but none of them are totally ideal.

1. Use the c libraries exit to exit immediately. This is not awesome because we don't allow qt to do any sort of cleanup, and it limits how phantom can do its own cleanup.

2. Use setjmp/longjmp. This is awful for readabililty, but also we because we assume too much about the internal details evaluateJavaScript.If we just prematurely exit the call we don't let it do any sort of cleanup that it might need to do.

3. Implement phantom::exit as a thrown exception. This is probably my favorite solution but it might break existing scripts that have generic exception handlers.

We may also just say that this is a wontfix considering if we can remove all the sideeffects what does it matter if "var a = 3;" will be evaluated.

[Ariya Hidayat]

This is indeed caused by https://github.com/ariya/phantomjs/commit/ecda224233.

I've reverted that one and handled potential crashes in a slightly
different way, based on Shawn's explanation.

Nicolas, can you check again if master is working for you? If yes,
then I'll bring this to 1.7 branch for some 1.7.1 release in the near
future.

Thanks!

[James Greene]

Huh... that's really interesting.  I assumed that `phantom.exit()` would indeed clean up as needed (e.g. memory references) and then exit the process without allowing any subsequent script code to execute.  I suppose this explains why I've been seeing multiple PhantomJS processes running during my builds (they are executed serially).

~~James

[Ariya Hidayat]

exit() will always exit and terminate the process (if that does not
happen, it's a different bug). It's just a question of when.

The crash was most of the time not really noticed, until we have
Breakpad which catches such a situation.

[James Greene]

That's correct: my processes do exit eventually, just not as early as I expected them to.  That's fine for my needs.

~~James

[Ariya Hidayat]

> Nicolas, can you check again if master is working for you? If yes,
> then I'll bring this to 1.7 branch for some 1.7.1 release in the near
> future.

I have some other small fixes in mind for 1.7.1. We may roll it out in
2-3 weeks.

Nicolas, if master works again for CasperJS, I'll backport the exit()
handling to 1.7 branch. Please confirm.


Thanks!

Regards,

[Nicolas Perriault]

On Fri, Sep 28, 2012 at 4:59 PM, Ariya Hidayat <ariya....@gmail.com> wrote:

> Nicolas, if master works again for CasperJS, I'll backport the exit()
> handling to 1.7 branch. Please confirm.

Just compiled a fresh master version of phantomjs, it now fully works
with casperjs. I'll now have to adjust the compatibility layer, but
that's another problem I'll be taking care of :)

Thanks!

++

[Ariya Hidayat]

> Just compiled a fresh master version of phantomjs, it now fully works
> with casperjs. I'll now have to adjust the compatibility layer, but
> that's another problem I'll be taking care of :)

I've merge master to 1.7 branch. The upcoming 1.7.1 should have the fixes.

[Ariya Hidayat]

> I've merge master to 1.7 branch. The upcoming 1.7.1 should have the fixes.

Unfortunately I don't have enough time to do 1.7.1, and of course 1.8
is just around the corner.

I did however cherry-pick a couple of important fixes (backported) to
1.7 branch, just in case someone wants to use it.


Regards,