phantom.exit() segmentation faults when followed by a phantom.injectJS

[telaviv]

Krisman....@gmail.com commented:

Which version of PhantomJS are you using?

1.7.0 (development)

What steps will reproduce the problem?
create a file "segfaulted-code.js" with the following code:
phantom.exit();
phantom.injectJs('hey!')

then run:
phantomjs segfaulted-code.js

What is the expected output? What do you see instead?
You see a segmentation fault, when it should exit cleanly

Which operating system are you using?

$:~/src/phantomjs/bin$ cat /etc/lsb-release
DISTRIB_ID=Ubuntu
DISTRIB_RELEASE=12.04
DISTRIB_CODENAME=precise
DISTRIB_DESCRIPTION="Ubuntu 12.04.1 LTS"

Did you use binary PhantomJS or did you compile it from source?
Compile from source.

Please provide any additional information below.
here is my gdb output:

(gdb) bt

0 WebPage::mainFrame (this=0x0) at webpage.cpp:325

1 0x000000000040e822 in Phantom::injectJs (this=0x28a3bd0, jsFilePath=...) at phantom.cpp:319

2 0x0000000000450d36 in qt_static_metacall (_a=<optimized out>, _id=<optimized out>, _o=<optimized out>, _c=<optimized out>) at moc_phantom.cpp:97

3 Phantom::qt_static_metacall (_o=<optimized out>, _c=<optimized out>, _id=<optimized out>, _a=<optimized out>) at moc_phantom.cpp:79

4 0x0000000000450ecf in Phantom::qt_metacall (this=0x28a3bd0, _c=QMetaObject::InvokeMetaMethod, _id=7, _a=0x7fffffffd910) at moc_phantom.cpp:143

5 0x00000000005225a5 in JSC::Bindings::QtRuntimeMetaMethod::call (exec=<optimized out>) at ../../WebCore/bridge/qt/qt_runtime.cpp:1455

6 0x0000000000e5a3a7 in JSC::cti_op_call_NotJSFunction (args=0x7fffffffdb00) at ../../JavaScriptCore/jit/JITStubs.cpp:2191

7 0x00007fffb004f6d5 in ?? ()

8 0x00007ffff7f74150 in ?? ()

9 0x00007ffff7f6b8e0 in ?? ()

10 0x00007fff0000000a in ?? ()

11 0x00007fff00000002 in ?? ()

12 0x00000000000002e6 in ?? ()

13 0x0000000001bfa5ce in QMetaObject::activate (sender=0x7ffff46d4078, m=<optimized out>, local_signal_index=<optimized out>, argv=0x7fffb004ed10)

at kernel/qobject.cpp:3547

14 0x00000000004cc044 in JSC::evaluate (exec=0x7ffff7f701d8, scopeChain=0x7ffff7f68150, source=..., thisValue=...) at ../../JavaScriptCore/runtime/Completion.cpp:64

15 0x0000000000500921 in evaluate (thisValue=..., source=..., chain=<optimized out>, exec=0x7ffff7f701d8) at ../../WebCore/bindings/js/JSMainThreadExecState.h:54

16 WebCore::ScriptController::evaluateInWorld (this=0x7ffff52ff818, sourceCode=..., world=<optimized out>) at ../../WebCore/bindings/js/ScriptController.cpp:143

17 0x0000000000501252 in WebCore::ScriptController::evaluate (this=0x7ffff52ff818, sourceCode=...) at ../../WebCore/bindings/js/ScriptController.cpp:167

18 0x00000000004efff0 in WebCore::ScriptController::executeScript (this=0x7ffff52ff818, sourceCode=...) at ../../WebCore/bindings/ScriptControllerBase.cpp:64

19 0x000000000045e56b in QWebFrame::evaluateJavaScript (this=0x28a3060, scriptSource=..., location=...) at Api/qwebframe.cpp:1556

20 0x0000000000425673 in Utils::injectJsInFrame (jsFilePath=..., jsFileEnc=..., libraryPath=..., targetFrame=0x28a3060, startingScript=true) at utils.cpp:111

21 0x000000000040dc6f in Phantom::execute (this=0x28a3bd0) at phantom.cpp:198

22 0x000000000040ce35 in main (argc=2, argv=<optimized out>, envp=0x7fffffffe2a0) at main.cpp:115

The underlying issue is that the call to Phantom::doExit prematurely deletes all the references to webPages. When the call to Phantom::injectJS happens, the webPage has already been deleted, so a segfault happens.

Disclaimer:
This issue was migrated on 2013-03-15 from the project's former issue tracker on Google Code, Issue #719.
🌟   3 people had starred this issue at the time of migration.

[telaviv]

Krisman....@gmail.com commented:

added a pull request here: #302

[telaviv]

Krisman....@gmail.com commented:

This reverts two commits:

5acaa6b
8094cdb

Both commits attempt to overcome a bug with QtWebKit where WebPages needed to be destroyed before our QtApplication was destroyed. If this bug were not there then our QObject relations would handle the necessary cleanup for us. I looked back on the issues that led to the fix in the first place which are #136 #148 and #149. I couldn't reproduce the issues at all. For instance:

$ phantomjs loadspeed.js http://dl.dropbox.com/u/9451381/tmp/test1.html

Should lead to a segmentation fault, but it doesn't for me. This leads me to believe (perhaps naively) that this was fixed upstream for us. So in theory we should be able to remove the hack that is commit 5acaa6b.

8094cdb alludes to wanting Phantom::exit to actually exit before running the rest of the code. I'm not sure if the intention is to have phantom.exit() be the last call in a script, but if it is then it currently isn't and we should create another issue for that.

[telaviv]

Krisman....@gmail.com commented:

As suggested by Ariya I tried to test my fix on windows. Unfortunately I can't get qt to compile. I was following the steps here: http://phantomjs.org/build.html. When I got to the step where I run preconfig.cmd, it runs for a while, but then errors out with:
"""
Generating Code...
lib /NOLOGO /OUT:........\plugins\codecs\qkrcodecs.lib @C:\Users\sean\AppData\Local\Temp\nmF1B4.tmp
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
Reading C:/Users/sean/workspace/phantomjs/src/qt/src/3rdparty/webkit/Source/WebKit/qt/QtWebKit.pro
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: Failure to find: plugins\win\PaintHooks.asm
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: c:\Users\sean\workspace\phantomjs\src\qt.qmake.cache:7: Unescaped backslashes are deprecated.
WARNING: Failure to find: plugins\win\PaintHooks.asm

Microsoft (R) Program Maintenance Utility Version 10.00.30319.01
Copyright (C) Microsoft Corporation. All rights reserved.

    cd WebKit\qt\ && "C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\amd64\nmake.exe" -f Makefile.QtWebKit

Microsoft (R) Program Maintenance Utility Version 10.00.30319.01
Copyright (C) Microsoft Corporation. All rights reserved.

    "C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\amd64\nmake.exe" -f Makefile.QtWebKit.Release

Microsoft (R) Program Maintenance Utility Version 10.00.30319.01
Copyright (C) Microsoft Corporation. All rights reserved.

NMAKE : fatal error U1073: don't know how to make 'plugins\win\PaintHooks.asm'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\amd64\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: 'cd' : return code '0x2'
Stop.
The system cannot find the file specified.
The system cannot find the file specified.
"""

There doesn't seem to be a directory called plugins/win

[ariya]

persianp...@gmail.com commented:

Shouldn't we be patching the cause rather than the symptoms? Phantomjs should not be running script code after phantom.exit.

For example, here's another segmentation fault: "phantom.exit(); require('webserver')". That segfault won't be fixed by this patch, right? Are we going to have a separate patch for each type of resource that could be accessed after phantom.exit?

See also https://code.google.com/p/phantomjs/issues/detail?id=444

[ariya]

ariya.hi...@gmail.com commented:

The build error is quite strange, I didn't have any problem with my VS 2010. Was it Pro edition or Express?

[telaviv]

Krisman....@gmail.com commented:

Hey Ariya

Im using VS 2010 Ultimate. Im still debugging the issue but it seems to get stranger and stranger. The first warnings happen when qmaking WebCore. After realizing what was going on, I see that in WebCore I totally have a plugins/win/PaintHooks.asm file. Its readable and seems fine. So i'm going to continue to work that out.

PersianP (lol google groups)

This patch actually fixes the segfault you wrote. The reason is because this is the only spot in the Phantom class where premature destruction happens. So if anything is going to segfault because of a null pointer, its likely to be here. I think even if we fix phantom.exit() this will be a big win because this is the kind of code that memory issues love to hang out.

So thanks for the link I suspected this might be affecting some people and I'm glad to see its documented and thought through already. Similar to the code i'm removing, non-terminating exit seems to only exist as a hack to make windows not segfault. If I can get the windows build to work then I would be happy to investigate making exit work again.

[telaviv]

Krisman....@gmail.com commented:

err sorry i mean't qmaking 3rdparty\webkit\Source

[ariya]

persianp...@gmail.com commented:

This patch actually fixes the segfault you wrote

Sounds good!

Sorry if I was dickish in my previous comment. I didn't want to see development effort wasted (like, if phantom.exit() is later fixed and it obviates your use-after-free fixes), but fixing the crashes should be #1 priority anyway so go for it.

I have no clue about the build errors unfortunately. People on the mailing list have been working on the Windows build process recently; you might find help there.

[telaviv]

Krisman....@gmail.com commented:

Cool the mailing list helped a lot. The problem above was caused by me trying a 64 bit windows build. 32 works as expected (mostly.) I finally got the chance to test the fix on windows and I can confirm that it works perfectly.

[telaviv]

Krisman....@gmail.com commented:

Also the old issues (#136 #148 and #149) still don't exist.

[ariya]

ariya.hi...@gmail.com commented:

Alessandro, what do you think? See #302.

 
Metadata Updates

• CC(s) added:
  • alessandro.portale@gmail.com
• Milestone updated: FutureRelease (was: ---)
• Status updated: Accepted

[ariya]

ariya.hi...@gmail.com commented:

What do you mean by "old issues (#136 #148 and #149) still don't exist."

[ariya]

ariya.hi...@gmail.com commented:

Revert "Fix crash on exit (Issues #136, #148 and #149)"
ecda224233

 
Metadata Updates

• Label(s) added:
  • Component-Logic
• Milestone updated: Release1.7 (was: FutureRelease)
• Status updated: Fixed

[telaviv]

Krisman....@gmail.com commented:

I mean to say that I tested issues 136, 148 and 149 to see if the removal of the code brought back the old issues. The issues didn't reappear on linux or on windows.

[telaviv]

Krisman....@gmail.com commented:

Unfortunately since my fix keeps webpages alive, many side effects that didn't uses to occur like printing to stdout post Phantom::exit now do. Until we figure out a way to prevent code running after Phantom::exit, we should keep prematurely deleting these webpages.

I created a new pull request that reverts my old change, and simply checks to make sure m_page exists before running additional js in its frames.

#322