This repository has been archived by the owner. It is now read-only.

onLongRunningScript and stopJavaScript do not work #12504

Open
ariya opened this Issue Aug 26, 2014 · 28 comments

Comments

Projects
None yet
7 participants
@ariya
Owner

ariya commented Aug 26, 2014

This is a regression on master branch, as reported by @zackw on the mailing-list: https://groups.google.com/d/msg/phantomjs-dev/yW-rsf9AIbU/O19HH-we2LoJ.

Test code:

    function expect(val, exp) { 
        if (val !== exp) 
            console.log('FAIL: ' + val + ' !== ' + exp); 
    } 

    var page = require('webpage').create(); 
    var longRunningScriptCalled = false; 
    var loadStatus; 

    page.onLongRunningScript = function() { 
        page.stopJavaScript(); 
        longRunningScriptCalled = true; 
    }; 
    page.onError = function (msg, trace) { 
        console.log('ERROR: ' + msg); 
    }; 

    page.open('data:text/html,' + 
              '<script>function forever() { while(true) {} }</script>' + 
              '<body onload="forever();"></body>', 
              function(status) { loadStatus = status; }); 

    setTimeout(function() { 
        expect(loadStatus, 'success'); 
        expect(longRunningScriptCalled, true); 
        phantom.exit(0); 
    }, 5000); 

@ariya ariya added this to the Release 2.0 milestone Aug 26, 2014

@ariya ariya added the Regression label Aug 26, 2014

@ariya ariya modified the milestones: Release 2.1, Release 2.0 Feb 1, 2015

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Feb 26, 2015

I've been digging around with this one... Might save you some time. I think It's a bug in webkit that is causing shouldInterruptJavaScript never to be called

Found this comment in tst_qtwebpage.cpp

// [Qt] tst_QWebPage::infiniteLoopJS() timeouts with DFG JIT
// https://bugs.webkit.org/show_bug.cgi?id=79040
// void infiniteLoopJS();

Bam4d commented Feb 26, 2015

I've been digging around with this one... Might save you some time. I think It's a bug in webkit that is causing shouldInterruptJavaScript never to be called

Found this comment in tst_qtwebpage.cpp

// [Qt] tst_QWebPage::infiniteLoopJS() timeouts with DFG JIT
// https://bugs.webkit.org/show_bug.cgi?id=79040
// void infiniteLoopJS();
@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Feb 26, 2015

this might also be an issue...
https://bugs.webkit.org/attachment.cgi?id=197273&action=prettypatch

looks like js timeouts are totally unsupported since 2013

Bam4d commented Feb 26, 2015

this might also be an issue...
https://bugs.webkit.org/attachment.cgi?id=197273&action=prettypatch

looks like js timeouts are totally unsupported since 2013

@mjgp2

This comment has been minimized.

Show comment
Hide comment
@mjgp2

mjgp2 Mar 18, 2015

@ariya - should this be closed then?

mjgp2 commented Mar 18, 2015

@ariya - should this be closed then?

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 18, 2015

I think we could implement the timeout (perhaps settable by the user) and then kill the phantom process if we hit the timeout for a single script? Not sure if there are hooks for things like script-start script-end etc.

Bam4d commented Mar 18, 2015

I think we could implement the timeout (perhaps settable by the user) and then kill the phantom process if we hit the timeout for a single script? Not sure if there are hooks for things like script-start script-end etc.

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Mar 18, 2015

Collaborator

@Bam4d My crawler does something like that and it sucks. The controller has to have all sorts of extra defensive code against garbage coming back from a PhantomJS process that got killed in the middle of producing its report.

@mjgp2 The last time I looked into this in any detail, there was a replacement mechanism inside JSC itself, but it was not fully wired up to be accessible from QtWebkit's external interface. It's not clear to me who needs to do what to get it done.

Collaborator

zackw commented Mar 18, 2015

@Bam4d My crawler does something like that and it sucks. The controller has to have all sorts of extra defensive code against garbage coming back from a PhantomJS process that got killed in the middle of producing its report.

@mjgp2 The last time I looked into this in any detail, there was a replacement mechanism inside JSC itself, but it was not fully wired up to be accessible from QtWebkit's external interface. It's not clear to me who needs to do what to get it done.

@jes

This comment has been minimized.

Show comment
Hide comment
@jes

jes Mar 18, 2015

looks like js timeouts are totally unsupported since 2013

Surely this doesn't mean browsers can be completely locked up by long-running scripts? Is this implemented by killing the whole child process?

I for one am strongly in favour of this feature being supported - I'm still using 1.9 due to the lack of it.

jes commented Mar 18, 2015

looks like js timeouts are totally unsupported since 2013

Surely this doesn't mean browsers can be completely locked up by long-running scripts? Is this implemented by killing the whole child process?

I for one am strongly in favour of this feature being supported - I'm still using 1.9 due to the lack of it.

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Mar 18, 2015

Collaborator

there was a replacement mechanism inside JSC itself

per my original report of this problem on the mailing list, see https://bugs.webkit.org/show_bug.cgi?id=114336 and https://bugs.webkit.org/show_bug.cgi?id=114577

Collaborator

zackw commented Mar 18, 2015

there was a replacement mechanism inside JSC itself

per my original report of this problem on the mailing list, see https://bugs.webkit.org/show_bug.cgi?id=114336 and https://bugs.webkit.org/show_bug.cgi?id=114577

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 18, 2015

@zackw yeah it looks like there's some sort of watchdog timer implemented. Will have to delve into it and see if there's a simple way to hook it into Qt...

Bam4d commented Mar 18, 2015

@zackw yeah it looks like there's some sort of watchdog timer implemented. Will have to delve into it and see if there's a simple way to hook it into Qt...

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 23, 2015

@zackw As a bit of an experiment i went through the code and tried to implement a hack for the WDT.
Here are the following steps I tried:

The API are currently kept as internal in JSContextRefPrivate.cpp

typedef bool
(*JSShouldTerminateCallback) (JSContextRef ctx, void* context);

JS_EXPORT void JSContextGroupSetExecutionTimeLimit(JSContextGroupRef, double limit, JSShouldTerminateCallback, void* context) AVAILABLE_IN_WEBKIT_VERSION_4_0;

JS_EXPORT void JSContextGroupClearExecutionTimeLimit(JSContextGroupRef) AVAILABLE_IN_WEBKIT_VERSION_4_0;

I moved them into the JSContextRef.cpp to expose them.

Implemented a bit of hack code into QWebFrameAdapter.cpp (just wanted to know if it was going to make anything happen.

bool terminateCallback(JSContextRef ctx, void* context)
{
    QWebFrameAdapter* frameAdapter = static_cast<QWebFrameAdapter*>(context);
    frameAdapter->evaluateJavaScript("console.log(\"callback terminated\");", "blah");

    return true;
}

void QWebFrameAdapter::setJSTimeout() {
    ScriptController* scriptController = frame->script();
    JSC::ExecState* exec = scriptController->globalObject(mainThreadNormalWorld())->globalExec();

    JSContextGroupRef contextGroup = toRef(&exec->vm());

    JSContextGroupSetExecutionTimeLimit(contextGroup, .1f, terminateCallback, this);
}

With gdb I can see that my callbacks are all fine and being set .. however delving a bit deeper into the code where the WDT actually gets set up... you come across something very disappointing:

startTimer() in WatchdogMac.cpp is implemented ... but Watchdog.cpp, is just totally empty. So the WDT code would only ever work on Mac (I'm developing for linux so I can't really test any further)
You know any good cross-platform timers we could stick in there?

I hope this information is useful in some way :)

Bam4d commented Mar 23, 2015

@zackw As a bit of an experiment i went through the code and tried to implement a hack for the WDT.
Here are the following steps I tried:

The API are currently kept as internal in JSContextRefPrivate.cpp

typedef bool
(*JSShouldTerminateCallback) (JSContextRef ctx, void* context);

JS_EXPORT void JSContextGroupSetExecutionTimeLimit(JSContextGroupRef, double limit, JSShouldTerminateCallback, void* context) AVAILABLE_IN_WEBKIT_VERSION_4_0;

JS_EXPORT void JSContextGroupClearExecutionTimeLimit(JSContextGroupRef) AVAILABLE_IN_WEBKIT_VERSION_4_0;

I moved them into the JSContextRef.cpp to expose them.

Implemented a bit of hack code into QWebFrameAdapter.cpp (just wanted to know if it was going to make anything happen.

bool terminateCallback(JSContextRef ctx, void* context)
{
    QWebFrameAdapter* frameAdapter = static_cast<QWebFrameAdapter*>(context);
    frameAdapter->evaluateJavaScript("console.log(\"callback terminated\");", "blah");

    return true;
}

void QWebFrameAdapter::setJSTimeout() {
    ScriptController* scriptController = frame->script();
    JSC::ExecState* exec = scriptController->globalObject(mainThreadNormalWorld())->globalExec();

    JSContextGroupRef contextGroup = toRef(&exec->vm());

    JSContextGroupSetExecutionTimeLimit(contextGroup, .1f, terminateCallback, this);
}

With gdb I can see that my callbacks are all fine and being set .. however delving a bit deeper into the code where the WDT actually gets set up... you come across something very disappointing:

startTimer() in WatchdogMac.cpp is implemented ... but Watchdog.cpp, is just totally empty. So the WDT code would only ever work on Mac (I'm developing for linux so I can't really test any further)
You know any good cross-platform timers we could stick in there?

I hope this information is useful in some way :)

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Mar 23, 2015

Collaborator

I just love it when OS vendors invent their very own completely non-portable API for something that there's a perfectly good POSIX API for. You're looking for timer_create. I would be inclined to use SIGEV_THREAD mode and pass the address of the Watchdog object into the callback -- it's expensive, but it means we don't have to muck around with signals.

(For this to work reliably, m_timerDidFire probably needs to be made into a std::atomic_flag and its sense inverted -- that is, the timer callback calls .clear() and the watchdog checker calls .test_and_set().)

Collaborator

zackw commented Mar 23, 2015

I just love it when OS vendors invent their very own completely non-portable API for something that there's a perfectly good POSIX API for. You're looking for timer_create. I would be inclined to use SIGEV_THREAD mode and pass the address of the Watchdog object into the callback -- it's expensive, but it means we don't have to muck around with signals.

(For this to work reliably, m_timerDidFire probably needs to be made into a std::atomic_flag and its sense inverted -- that is, the timer callback calls .clear() and the watchdog checker calls .test_and_set().)

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Mar 23, 2015

Collaborator

I may have time to code this up next week, if you don't get to it first, but not before then. I will be happy to review patches at any time.

Collaborator

zackw commented Mar 23, 2015

I may have time to code this up next week, if you don't get to it first, but not before then. I will be happy to review patches at any time.

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 23, 2015

I'm pretty confused as to why there are three Watchdog files, not familiar with this build process at all...
Watchdog.cpp - boilerplate
WatchdogMac.cpp - the mac stuff
WatchdogNone.cpp - the empty shims

I guess the compiler is combining these somehow based on the platform and the suffixes in the file names

Bam4d commented Mar 23, 2015

I'm pretty confused as to why there are three Watchdog files, not familiar with this build process at all...
Watchdog.cpp - boilerplate
WatchdogMac.cpp - the mac stuff
WatchdogNone.cpp - the empty shims

I guess the compiler is combining these somehow based on the platform and the suffixes in the file names

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 23, 2015

My work in progress is happening on a branch in a fork so far, any comments would be appreciated.

edit: it works! just need to make it look nicer and hook the callback into phantom.

Bam4d commented Mar 23, 2015

My work in progress is happening on a branch in a fork so far, any comments would be appreciated.

edit: it works! just need to make it look nicer and hook the callback into phantom.

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Mar 23, 2015

Collaborator

@Bam4d I don't understand the build process either, but based on the structure of the C++, it must be compiling Watchdog.cpp for all platforms, WatchdogMac.cpp for OSX only, and WatchdogNone.cpp on not-OSX platforms. (There is nothing in the C++ spec saying you can't split the implementation of one class over multiple files, it's just not done very often.)

Collaborator

zackw commented Mar 23, 2015

@Bam4d I don't understand the build process either, but based on the structure of the C++, it must be compiling Watchdog.cpp for all platforms, WatchdogMac.cpp for OSX only, and WatchdogNone.cpp on not-OSX platforms. (There is nothing in the C++ spec saying you can't split the implementation of one class over multiple files, it's just not done very often.)

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Mar 23, 2015

Collaborator

@Bam4d ... so based on that, I think what you want is to create a WatchdogPosix.cpp that just implements the functions that are stubbed in WatchdogNone.cpp; leave WatchdogNone.cpp alone; have the conditionals in Watchdog.h add a timer_t to the object if !PLATFORM(WIN); and adjust Source/JavaScriptCore/Target.pri to include the appropriate watchdog implementation instead of unconditionally using WatchdogNone as it does now. (I think that's the relevant build file. I am thoroughly unimpressed with how much of a mess the Webkit source tree is.)

Collaborator

zackw commented Mar 23, 2015

@Bam4d ... so based on that, I think what you want is to create a WatchdogPosix.cpp that just implements the functions that are stubbed in WatchdogNone.cpp; leave WatchdogNone.cpp alone; have the conditionals in Watchdog.h add a timer_t to the object if !PLATFORM(WIN); and adjust Source/JavaScriptCore/Target.pri to include the appropriate watchdog implementation instead of unconditionally using WatchdogNone as it does now. (I think that's the relevant build file. I am thoroughly unimpressed with how much of a mess the Webkit source tree is.)

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 25, 2015

Did some more work on this today... the WatchdogMac.cpp Isn't even referenced anywhere, so I don't actually think it works on Mac, even though the code is there?!

Anyway I created a WatchdogPosix.cpp, changed the Target.pri and hooked in some callbacks up to shouldInterruptJavaScript

Works nicely, but when I'm running the tests, it hangs after running the associated test. Can't see any errors being reported from gdb, so might need some help on this. Am I missing something obvious?

branch

Bam4d commented Mar 25, 2015

Did some more work on this today... the WatchdogMac.cpp Isn't even referenced anywhere, so I don't actually think it works on Mac, even though the code is there?!

Anyway I created a WatchdogPosix.cpp, changed the Target.pri and hooked in some callbacks up to shouldInterruptJavaScript

Works nicely, but when I'm running the tests, it hangs after running the associated test. Can't see any errors being reported from gdb, so might need some help on this. Am I missing something obvious?

branch

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Mar 26, 2015

I -think- that when the script is being killed, the rest of the event callbacks to phantom aren't happening, page loaded etc.. doesn't explain why the test freezes, I would assume the test would just fail rather than freeze?

Bam4d commented Mar 26, 2015

I -think- that when the script is being killed, the rest of the event callbacks to phantom aren't happening, page loaded etc.. doesn't explain why the test freezes, I would assume the test would just fail rather than freeze?

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Aug 25, 2015

Collaborator

I still don't know why it hangs. But it hangs on creating a WebPage instance.

Are you sure it hangs on the create() and not the open()? Patch 2/2 doesn't work yet and that would cause it to hang on the open() even with a good Windows watchdog. (I got all wound up in other work today and didn't get back to it. Tomorrow for sure!~)

Collaborator

zackw commented Aug 25, 2015

I still don't know why it hangs. But it hangs on creating a WebPage instance.

Are you sure it hangs on the create() and not the open()? Patch 2/2 doesn't work yet and that would cause it to hang on the open() even with a good Windows watchdog. (I got all wound up in other work today and didn't get back to it. Tomorrow for sure!~)

@Vitallium

This comment has been minimized.

Show comment
Hide comment
@Vitallium

Vitallium Aug 25, 2015

Collaborator

Yes, I'm sure. I even can't get to the open() under a debugger.

Collaborator

Vitallium commented Aug 25, 2015

Yes, I'm sure. I even can't get to the open() under a debugger.

@Bam4d

This comment has been minimized.

Show comment
Hide comment
@Bam4d

Bam4d Aug 28, 2015

My thoughts on this back when I was working on it, was that the hang was happening because there's a callback that should be getting called (to exit the interpreter loop) and complete the parsing of the page. Been far too long since I looked at this code.

Bam4d commented Aug 28, 2015

My thoughts on this back when I was working on it, was that the hang was happening because there's a callback that should be getting called (to exit the interpreter loop) and complete the parsing of the page. Been far too long since I looked at this code.

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Aug 28, 2015

Collaborator

@Vitallium The hang does not happen on the create for me with that script.

@Vitallium @ariya How is page.settings supposed to work exactly? Changes to page.settings.whatever don't cause WebPage::applySettings to get called and I can't figure out why.

Collaborator

zackw commented Aug 28, 2015

@Vitallium The hang does not happen on the create for me with that script.

@Vitallium @ariya How is page.settings supposed to work exactly? Changes to page.settings.whatever don't cause WebPage::applySettings to get called and I can't figure out why.

zackw added a commit to zackw/phantomjs that referenced this issue Aug 29, 2015

JSC script watchdog implementation for POSIX-compliant OSes.
This implementation should work on any OS that provides
POSIX.1 interval timers (timer_create, _delete, _settime).

Also, fix some minor bugs in the generic and MacOS-specific
watchdog code, and enable WatchdogMac.cpp in the MacOS build.

Part of PhantomJS issue #12504.

zackw added a commit to zackw/phantomjs that referenced this issue Aug 29, 2015

Add page.settings.scriptTimeout and plumb it through to JSC.
The underlying JavaScriptCore mechanism for interrupting scripts
has changed, and requires all-new glue in the QtWebKit layer.

PhantomJS issue #12504.

zackw added a commit to zackw/phantomjs that referenced this issue Oct 20, 2015

JSC script watchdog implementation for POSIX-compliant OSes.
This implementation should work on any OS that provides
POSIX.1 interval timers (timer_create, _delete, _settime).

Also, fix some minor bugs in the generic and MacOS-specific
watchdog code, and enable WatchdogMac.cpp in the MacOS build.

Part of PhantomJS issue #12504.

zackw added a commit to zackw/phantomjs that referenced this issue Oct 20, 2015

Add page.settings.scriptTimeout and plumb it through to JSC.
The underlying JavaScriptCore mechanism for interrupting scripts
has changed, and requires all-new glue in the QtWebKit layer.

PhantomJS issue #12504.

zackw added a commit to zackw/phantomjs that referenced this issue Nov 3, 2015

JSC script watchdog implementation for POSIX-compliant OSes.
This implementation should work on any OS that provides
POSIX.1 interval timers (timer_create, _delete, _settime).

Also, fix some minor bugs in the generic and MacOS-specific
watchdog code, and enable WatchdogMac.cpp in the MacOS build.

Part of PhantomJS issue #12504.

zackw added a commit to zackw/phantomjs that referenced this issue Nov 3, 2015

Add page.settings.scriptTimeout and plumb it through to JSC.
The underlying JavaScriptCore mechanism for interrupting scripts
has changed, and requires all-new glue in the QtWebKit layer.

PhantomJS issue #12504.

@zackw zackw modified the milestones: Release 2.1, Release 2.2, FutureRelease Feb 3, 2016

@madelson

This comment has been minimized.

Show comment
Hide comment
@madelson

madelson Feb 26, 2016

Has this been fixed? If not, what is it's status?

madelson commented Feb 26, 2016

Has this been fixed? If not, what is it's status?

@zackw zackw added help-wanted and removed In progress labels Feb 26, 2016

@zackw

This comment has been minimized.

Show comment
Hide comment
@zackw

zackw Feb 26, 2016

Collaborator

I gave up on it. Someone who's better at Qt+Webkit internals might be able to get the existing patches to work, but I wasn't getting anywhere.

Collaborator

zackw commented Feb 26, 2016

I gave up on it. Someone who's better at Qt+Webkit internals might be able to get the existing patches to work, but I wasn't getting anywhere.

@Vitallium Vitallium removed the Need design label Oct 1, 2016

@ghost ghost removed this from the FutureRelease milestone Jan 10, 2018

@ghost ghost removed 2.0 Regression labels Jan 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.