Defer ScriptExecutionContext::Task's in Document when page loading is deferred

[Yong Li]

2 reasons why we should do this:

1) when page loading is deferred, we are not supposed to execute JS.

2) Currently, if we perform the tasks when page loading is deferred, those JS callbacks will be discarded, because ActiveDOMObjects are all suspended.

[Yong Li]

Created attachment 73654 [details]
the patch

[Yong Li]

Comment on attachment 73654 [details]
the patch

test case should be rewritten

[WebKit Review Bot]

Attachment 73654 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/dom/Document.cpp', u'WebCore/dom/Document.h', u'WebCore/manual-tests/database-callback-deferred.html', u'WebCore/page/PageGroupLoadDeferrer.cpp']" exit_code: 1
WebCore/dom/Document.cpp:592:  Missing space before ( in for(  [whitespace/parens] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yong Li]

Created attachment 73657 [details]
updated

[Yong Li]

Created attachment 73658 [details]
fix style

[WebKit Review Bot]

Attachment 73657 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/dom/Document.cpp', u'WebCore/dom/Document.h', u'WebCore/manual-tests/database-callback-deferred.html', u'WebCore/page/PageGroupLoadDeferrer.cpp']" exit_code: 1
WebCore/dom/Document.cpp:592:  Missing space before ( in for(  [whitespace/parens] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Attachment 73654 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5702008

[Eric Seidel (no email)]

Attachment 73658 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5670006

[Eric Seidel (no email)]

Attachment 73658 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5771004

[Yong Li]

Created attachment 74385 [details]
remove an unused variable in test case

[Eric Seidel (no email)]

Attachment 74385 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6229071

[Eric Seidel (no email)]

Attachment 74385 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6197085

[Eric Seidel (no email)]

Attachment 74385 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6204053

[Eric Seidel (no email)]

Comment on attachment 74385 [details]
remove an unused variable in test case

Would break mac.

[Yong Li]

Created attachment 76389 [details]
try again

[WebKit Review Bot]

Attachment 76389 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6984090

[Eric Seidel (no email)]

Attachment 76389 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6891121

[Eric Seidel (no email)]

Attachment 76389 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7042024

[Yong Li]

Created attachment 76407 [details]
finally found the warning that fails build

[Darin Adler]

Comment on attachment 76407 [details]
finally found the warning that fails build

View in context: https://bugs.webkit.org/attachment.cgi?id=76407&action=review

> WebCore/dom/Document.cpp:600
> +    for (size_t i = 0; i < m_pendingTasks.size(); ++i)
> +        delete m_pendingTasks[i];

There’s a function for this called deleteAllValues.

But more importantly, Vector<OwnPtr> works just fine, so we should use it here rather than ll those leakPtr calls.

> WebCore/dom/Document.cpp:4696
> -static void performTask(void* ctx)
> +void Document::didReceiveTask(void* ctx)

Lets use a word here instead of the “ctx” abbreviation. Maybe contextPtr? Yes, that uses an abbrevation too, so I may be a hypocrite.

> WebCore/dom/Document.cpp:4700
> +    OwnPtr<PerformTaskContext> context(reinterpret_cast<PerformTaskContext*>(ctx));

This should be using adoptPtr and should use static_cast rather than reinterpret_cast.

> WebCore/dom/Document.cpp:4728
> +        OwnPtr<Task> task(m_pendingTasks[0]);

This should use adoptPtr, but would actually use release if it was a Vector<OwnPtr>.

Since we use this by adding to the end and removing from the start, this should be a Deque rather than a Vector. If it was a Deque we could probably use the takeFirst function rather than writing it like this.

> WebCore/dom/Document.cpp:4738
> +    suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog);
> +    asyncScriptRunner()->suspend();
> +    m_pendingTasksTimer.stop();

Perhaps suspendActiveDOMObjects could be made private now?

> WebCore/dom/Document.cpp:4746
> +    resumeActiveDOMObjects();

Perhaps resumeActiveDOMObjects could be made private now?

> WebCore/dom/Document.h:1067
> +    virtual void willDeferLoading();
> +    virtual void didResumeLoading();

Why are these functions virtual?

It would be better if these names paired up a bit better.

> WebCore/page/PageGroupLoadDeferrer.cpp:50
> -                for (Frame* frame = otherPage->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
> -                    frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog);
> -                    frame->document()->asyncScriptRunner()->suspend();
> -                }
> +                for (Frame* frame = otherPage->mainFrame(); frame; frame = frame->tree()->traverseNext())
> +                    frame->document()->willDeferLoading();

Each frame already gets a call about deferring loading in response to the Page::setDefersLoading call. Specifically, FrameLoader::setDefersLoading. Could we put the responsibility for calling these functions there? Is there some other call site that calls setDefersLoading but does not want this additional work to be done?

I find the willDefer and didResume names a little confusing and this is existing code is strangely disconnected from the main setDefersLoading feature.

[Yong Li]

(In reply to comment #20)

Many cool ideas. Working on another patch.

suspendActiveDOMObjects cannot be private because it is from parent class: ScriptExecutionContext

[Yong Li]

Created attachment 76435 [details]
updated

cannot use Deque<OwnPtr<T> >

[Eric Seidel (no email)]

Attachment 76435 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6993098

[Yong Li]

(In reply to comment #23)
> Attachment 76435 [details] did not build on chromium:
> Build output: http://queues.webkit.org/results/6993098

I don't think it is caused by this patch. Where can I find the detailed error message?

[Eric Seidel (no email)]

You can re-post the patch, we recently fixed a logging problem with the mac-based EWS bots.  Hopefully they'll show errors better now.

[Yong Li]

Created attachment 76562 [details]
rerun builtbot

[Darin Adler]

Comment on attachment 76562 [details]
rerun builtbot

View in context: https://bugs.webkit.org/attachment.cgi?id=76562&action=review

Change looks OK, but this is a super-tricky area; I’d prefer it if we had more tests.

> WebCore/dom/Document.cpp:4693
> +void Document::didReceiveTask(void* rawContext)

I used the name “untypedContext” for this when I recently wrote some code using the same idiom. Your name is good too. Just wanted to share mine.

> WebCore/dom/Document.cpp:4710
> +        document->m_pendingTasks.append(context->task.leakPtr());

The leakPtr here is not what you want. You want to call release here instead. This only compiles because of LOOSE_OWN_PTR.

> WebCore/dom/Document.cpp:4728
> +    while (!m_pendingTasks.isEmpty()) {
> +        OwnPtr<Task> task = m_pendingTasks[0].release();
> +        m_pendingTasks.remove(0);
> +        task->performTask(this);
> +    }

This function is an argument for using the Deque class. Deque has a takeFirst function that is perfect for idioms like this.

> WebCore/loader/FrameLoader.cpp:260
> +    // This code is not logically part of load deferring, but we do not want JS code executed beneath modal
> +    // windows or sheets, which is exactly when PageGroupLoadDeferrer is used.
> +    // NOTE: if PageGroupLoadDeferrer is ever used for tasks other than showing a modal window or sheet,

It no longer makes sense to mention PageGroupLoadDeferrer here. This comment is now far enough away from PageGroupLoadDeferrer that people will not see the comment.

The word “if” should be capitalized here at the start of a sentence.

> WebCore/loader/FrameLoader.cpp:265
> +    if (defers)
> +        m_frame->document()->suspendScheduledTasks();
> +    else
> +        m_frame->document()->resumeScheduledTasks();

What’s the guarantee that we won’t change the document of a frame while suspended? If we don’t have that guarantee we could end up calling suspendScheduledTasks on a document and never calling resumeScheduledTasks on it.

[Yong Li]

(In reply to comment #27)
> (From update of attachment 76562 [details])
> > WebCore/dom/Document.cpp:4710
> > +        document->m_pendingTasks.append(context->task.leakPtr());
> The leakPtr here is not what you want. You want to call release here instead. This only compiles because of LOOSE_OWN_PTR.

forgot that I'm using Vector<OwnPtr> now.

> > WebCore/dom/Document.cpp:4728
> > +    while (!m_pendingTasks.isEmpty()) {
> > +        OwnPtr<Task> task = m_pendingTasks[0].release();
> > +        m_pendingTasks.remove(0);
> > +        task->performTask(this);
> > +    }
> This function is an argument for using the Deque class. Deque has a takeFirst function that is perfect for idioms like this.

I tried Deque<OwnPtr>, but it doesn't compile. Probably it is worth reporting another bug.

> > WebCore/loader/FrameLoader.cpp:265
> > +    if (defers)
> > +        m_frame->document()->suspendScheduledTasks();
> > +    else
> > +        m_frame->document()->resumeScheduledTasks();
> What’s the guarantee that we won’t change the document of a frame while suspended? If we don’t have that guarantee we could end up calling suspendScheduledTasks on a document and never calling resumeScheduledTasks on it.

From my understanding, when loading is deferred (for example a modal dialog is up), frame should never change its document except there is another bug, because if it can open new document, it must also be able to run JS, load resources, ...

[Yong Li]

Created attachment 77150 [details]
updated

[WebKit Commit Bot]

Comment on attachment 77150 [details]
updated

Rejecting attachment 77150 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--no-update', '--non-interactive', 77150]" exit_code: 2
Last 500 characters of output:
 with fuzz 2 (offset 5 lines).
Hunk #2 succeeded at 1137 (offset -2 lines).
Hunk #3 succeeded at 1388 (offset -3 lines).
patching file WebCore/loader/FrameLoader.cpp
patching file WebCore/manual-tests/database-callback-deferred.html
patching file WebCore/page/PageGroupLoadDeferrer.cpp
Hunk #1 FAILED at 40.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/page/PageGroupLoadDeferrer.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7303085

[Yong Li]

Created attachment 77919 [details]
fix merge error

[Yong Li]

Created attachment 77920 [details]
the right one

[WebKit Commit Bot]

Comment on attachment 77920 [details]
the right one

Rejecting attachment 77920 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'apply-attachment', '--no-update', '--non-interactive', 77920]" exit_code: 2
Last 500 characters of output:
bCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/dom/Document.cpp
patching file WebCore/dom/Document.h
patching file WebCore/loader/FrameLoader.cpp
patching file WebCore/manual-tests/database-callback-deferred.html
patching file WebCore/page/PageGroupLoadDeferrer.cpp
Hunk #1 FAILED at 40.
1 out of 1 hunk FAILED -- saving rejects to file WebCore/page/PageGroupLoadDeferrer.cpp.rej

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7266419

[Yong Li]

Created attachment 78032 [details]
try again

[WebKit Commit Bot]

Comment on attachment 78032 [details]
try again

Rejecting attachment 78032 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
....................................
fast/text/whitespace ..........................................
fast/tokenizer ........................................
fast/transforms ........................
fast/url ....................
fast/workers .........................
fast/workers/worker-close-more.html -> failed

Exiting early after 1 failures. 16401 tests run.
320.18s total testing time

16400 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
6 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7200441

[Eric Seidel (no email)]

Comment on attachment 78032 [details]
try again

It's possible that simply flaked twice.

[WebKit Commit Bot]

Comment on attachment 78032 [details]
try again

Rejecting attachment 78032 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
....................................
fast/text/whitespace ..........................................
fast/tokenizer ........................................
fast/transforms ........................
fast/url ....................
fast/workers .........................
fast/workers/worker-close-more.html -> failed

Exiting early after 1 failures. 16401 tests run.
339.88s total testing time

16400 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
7 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7362002

[Yong Li]

Created attachment 78117 [details]
ertry

[Yong Li]

(In reply to comment #36)
> (From update of attachment 78032 [details])
> It's possible that simply flaked twice.

seems it is a real issue. where can I find the actual result?

[Yong Li]

Created attachment 78129 [details]
Change the expected result of worker-close-more test

The only change is to add this line to the expected result of worker-close-more test:

CONSOLE MESSAGE: line 31: JavaScript execution exceeded timeout

This message is caused by the 31st line in worker-close.js: while (true) { }

So I think it was just missed and now we can catch it.

However I haven't been able to see the result diff on the bots... not sure if it is the same story.

[Eric Seidel (no email)]

Comment on attachment 76562 [details]
rerun builtbot

Cleared Darin Adler's review+ from obsolete attachment 76562 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Yong Li]

Comment on attachment 78129 [details]
Change the expected result of worker-close-more test

I'll update the patch because the files are moved to Source now

[Yong Li]

Created attachment 82647 [details]
Based on latest code

[Yong Li]

Comment on attachment 82647 [details]
Based on latest code

still fail the test

[Yong Li]

Created attachment 82660 [details]
Let commit log test the patch

[Yong Li]

Comment on attachment 82660 [details]
Let commit log test the patch

Darnn. still fail the test

[WebKit Commit Bot]

Comment on attachment 82647 [details]
Based on latest code

Rejecting attachment 82647 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2

Last 500 characters of output:
t.h
Hunk #1 succeeded at 1119 with fuzz 2 (offset 10 lines).
Hunk #2 succeeded at 1168 (offset 10 lines).
Hunk #3 FAILED at 1431.
1 out of 3 hunks FAILED -- saving rejects to file Source/WebCore/dom/Document.h.rej
patching file Source/WebCore/loader/FrameLoader.cpp
patching file Source/WebCore/manual-tests/database-callback-deferred.html
patching file Source/WebCore/page/PageGroupLoadDeferrer.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8084426

[Yong Li]

Created attachment 84630 [details]
up-to-date

[Yong Li]

Created attachment 84950 [details]
Try again

[WebKit Commit Bot]

Comment on attachment 84950 [details]
Try again

Clearing flags on attachment: 84950

Committed r80478: <http://trac.webkit.org/changeset/80478>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/80478 might have broken Qt Linux Release

[Csaba Osztrogonác]

(In reply to comment #52)
> http://trac.webkit.org/changeset/80478 might have broken Qt Linux Release

buildfix landed in http://trac.webkit.org/changeset/80480

[Ryosuke Niwa]

It seems like 16 inspector tests have been crashing sine this patch is landed:
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/20099
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r80480%20(20099)/results.html

[Martin Robinson]

Here's what I believe is the stack for these crashes:


#0  0x00007fdb4d7c11b6 in WebCore::SuspendableTimer::suspend (this=0x7fdb3c003ab0) at ../../Source/WebCore/page/SuspendableTimer.cpp:62
62	    ASSERT(!m_suspended);

#1  0x00007fdb4d43be0a in WebCore::ScriptExecutionContext::suspendActiveDOMObjects (this=0x7fdb3c00a808, why=WebCore::ActiveDOMObject::JavaScriptDebuggerPaused) at ../../Source/WebCore/dom/ScriptExecutionContext.cpp:247
#2  0x00007fdb4d233040 in WebCore::ScriptDebugServer::setJavaScriptPaused (this=0x20d9660, frame=0x1ade000, paused=true) at ../../Source/WebCore/bindings/js/ScriptDebugServer.cpp:413
#3  0x00007fdb4d232f38 in WebCore::ScriptDebugServer::setJavaScriptPaused (this=0x20d9660, page=0x1ad1ac0, paused=true) at ../../Source/WebCore/bindings/js/ScriptDebugServer.cpp:398
#4  0x00007fdb4d232e5f in WebCore::ScriptDebugServer::setJavaScriptPaused (this=0x20d9660, pageGroup=..., paused=true) at ../../Source/WebCore/bindings/js/ScriptDebugServer.cpp:388
#5  0x00007fdb4d233542 in WebCore::ScriptDebugServer::pauseIfNeeded (this=0x20d9660, page=0x1ad1ac0) at ../../Source/WebCore/bindings/js/ScriptDebugServer.cpp:478
#6  0x00007fdb4d2333a9 in WebCore::ScriptDebugServer::updateCallFrameAndPauseIfNeeded (this=0x20d9660, debuggerCallFrame=..., sourceID=40408256, lineNumber=10) at ../../Source/WebCore/bindings/js/ScriptDebugServer.cpp:454
#7  0x00007fdb4d233980 in WebCore::ScriptDebugServer::didReachBreakpoint (this=0x20d9660, debuggerCallFrame=..., sourceID=40408256, lineNumber=10) at ../../Source/WebCore/bindings/js/ScriptDebugServer.cpp:560
#8  0x00007fdb4df40350 in JSC::Interpreter::debug (this=0x20d82d0, callFrame=0x7fdafb9fd038, debugHookID=JSC::DidReachBreakpoint, firstLine=10, lastLine=10) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:1186
#9  0x00007fdb4df7a4a5 in JSC::cti_op_debug (args=0x7fff74a467d0) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:3458
#10 0x00007fdb4df6e6a3 in JSC::JITThunks::tryCacheGetByID (callFrame=0x7fff74a467d0, codeBlock=0x7fff74a467d0, returnAddress=..., baseValue=..., propertyName=..., slot=..., stubInfo=0x1abbe00) at ../../Source/JavaScriptCore/jit/JITStubs.cpp:869
#11 0x00007fdb4df41683 in JSC::JITCode::execute (this=0x256a128, registerFile=0x20d82e8, callFrame=0x7fdafb9fd038, globalData=0x1abbe00) at ../../Source/JavaScriptCore/jit/JITCode.h:77
#12 0x00007fdb4df3e753 in JSC::Interpreter::executeCall (this=0x20d82d0, callFrame=0x21f8408, function=0x7fdb4fb48d90, callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/interpreter/Interpreter.cpp:844
#13 0x00007fdb4dfcb898 in JSC::call (exec=0x21f8408, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/JavaScriptCore/runtime/CallData.cpp:38
#14 0x00007fdb4d1c36c7 in WebCore::JSMainThreadExecState::call (exec=0x21f8408, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../Source/WebCore/bindings/js/JSMainThreadExecState.h:48
#15 0x00007fdb4d223aca in WebCore::ScheduledAction::executeFunctionInContext (this=0x2f2d050, globalObject=0x7fdafb178c78, thisValue=..., context=0x7fdb3c00a808) at ../../Source/WebCore/bindings/js/ScheduledAction.cpp:106
#16 0x00007fdb4d223cbc in WebCore::ScheduledAction::execute (this=0x2f2d050, document=0x7fdb3c00a7a0) at ../../Source/WebCore/bindings/js/ScheduledAction.cpp:128
#17 0x00007fdb4d223886 in WebCore::ScheduledAction::execute (this=0x2f2d050, context=0x7fdb3c00a808) at ../../Source/WebCore/bindings/js/ScheduledAction.cpp:76
#18 0x00007fdb4d7520d1 in WebCore::DOMTimer::fired (this=0x2ecc510) at ../../Source/WebCore/page/DOMTimer.cpp:130
#19 0x00007fdb4d879718 in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x1b06c00) at ../../Source/WebCore/platform/ThreadTimers.cpp:112
#20 0x00007fdb4d87964f in WebCore::ThreadTimers::sharedTimerFired () at ../../Source/WebCore/platform/ThreadTimers.cpp:90
#21 0x00007fdb4d05b0fa in WebCore::timeout_cb () at ../../Source/WebCore/platform/gtk/SharedTimerGtk.cpp:49
#22 0x00007fdb4a25fdbb in g_timeout_dispatch (source=0x2712720, callback=0, user_data=0x7fdb490d5e00) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3877
#23 0x00007fdb4a25f362 in g_main_dispatch (context=0xffff000000000002) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:2440
#24 g_main_context_dispatch (context=0xffff000000000002) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3013
#25 0x00007fdb4a263a28 in g_main_context_iterate (context=0x1a52760, block=<value optimized out>, dispatch=<value optimized out>, self=<value optimized out>) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3091
#26 0x00007fdb4a263f35 in g_main_loop_run (loop=0x7fdb3c001f40) at /tmp/buildd/glib2.0-2.27.91/./glib/gmain.c:3299
#27 0x00007fdb4c198657 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#28 0x000000000041e485 in runTest (testPathOrURL=...) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:679
#29 0x000000000041db17 in runTestingServerLoop () at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:489
#30 0x000000000041fbfc in main (argc=2, argv=0x7fff74a479e8) at ../../Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1143

[Philippe Normand]

Is someone looking at the inspector crashes caused by this patch?

[Pavel Feldman]

This change broke inspector debugger tests on the Leopard Debug Tests bot. 

#0  WebCore::SuspendableTimer::suspend (this=0x1196c26a0) at Source/WebCore/page/SuspendableTimer.cpp:62
#1  0x0000000101b349cf in WebCore::ScriptExecutionContext::suspendActiveDOMObjects (this=0x1073cc668, why=WebCore::ActiveDOMObject::WillShowDialog) at Source/WebCore/dom/ScriptExecutionContext.cpp:247
#2  0x00000001011c13fc in WebCore::Document::suspendScheduledTasks (this=0x1073cc600) at Source/WebCore/dom/Document.cpp:4758
#3  0x00000001013785ae in WebCore::FrameLoader::setDefersLoading (this=0x10607e8b8, defers=true) at Source/WebCore/loader/FrameLoader.cpp:260
#4  0x00000001019187c1 in WebCore::Page::setDefersLoading (this=0x105e83e10, defers=true) at Source/WebCore/page/Page.cpp:549
#5  0x0000000101b276fc in WebCore::ScriptDebugServer::setJavaScriptPaused (this=0x105ed9920, page=0x105e83e10, paused=true) at Source/WebCore/bindings/js/ScriptDebugServer.cpp:395
#6  0x0000000101b277ad in WebCore::ScriptDebugServer::setJavaScriptPaused (this=0x105ed9920, pageGroup=@0x105e6b280, paused=true) at Source/WebCore/bindings/js/ScriptDebugServer.cpp:388
#7  0x0000000101b2835f in WebCore::ScriptDebugServer::pauseIfNeeded (this=0x105ed9920, page=0x105e83e10) at Source/WebCore/bindings/js/ScriptDebugServer.cpp:478
#8  0x0000000101b28501 in WebCore::ScriptDebugServer::updateCallFrameAndPauseIfNeeded (this=0x105ed9920, debuggerCallFrame=@0x7fff5fbfe0e0, sourceID=4407861248, lineNumber=13) at Source/WebCore/bindings/js/ScriptDebugServer.cpp:454
#9  0x0000000101b2854d in WebCore::ScriptDebugServer::didReachBreakpoint (this=0x105ed9920, debuggerCallFrame=@0x7fff5fbfe0e0, sourceID=4407861248, lineNumber=13) at Source/WebCore/bindings/js/ScriptDebugServer.cpp:560
#10 0x00000001001bfb20 in JSC::Interpreter::debug (this=0x106b2e9c0, callFrame=0x1186bc038, debugHookID=JSC::DidReachBreakpoint, firstLine=13, lastLine=13) at Interpreter.cpp:1186
#11 0x00000001001e82e7 in cti_op_debug (args=0x7fff5fbfe1c0) at JITStubs.cpp:3460
#12 0x00000001001e73ef in WTF::doubleHash (key=Could not find the frame base for "WTF::doubleHash(unsigned int)".
) at HashTable.h:447
#13 0x00000001001c58b1 in JSC::JITCode::execute (this=0x106fff9b8, registerFile=0x106b2e9d8, callFrame=0x1186bc038, globalData=0x107026000) at JITCode.h:77
#14 0x00000001001c055a in JSC::Interpreter::executeCall (this=0x106b2e9c0, callFrame=0x11d1967b8, function=0x118e8f180, callType=JSC::CallTypeJS, callData=@0x7fff5fbfe5e0, thisValue={m_ptr = 0x118e73440}, args=@0x7fff5fbfe5d0) at Interpreter.cpp:844
#15 0x000000010017b9b1 in JSC::call (exec=0x11d1967b8, functionObject={m_ptr = 0x118e8f180}, callType=JSC::CallTypeJS, callData=@0x7fff5fbfe5e0, thisValue={m_ptr = 0x118e73440}, args=@0x7fff5fbfe5d0) at CallData.cpp:38
#16 0x0000000101cfa451 in WebCore::JSMainThreadExecState::call (exec=0x11d1967b8, functionObject={m_ptr = 0x118e8f180}, callType=JSC::CallTypeJS, callData=@0x7fff5fbfe5e0, thisValue={m_ptr = 0x118e73440}, args=@0x7fff5fbfe5d0) at JSMainThreadExecState.h:48
#17 0x0000000101b18884 in WebCore::ScheduledAction::executeFunctionInContext (this=0x11a4723d0, globalObject=0x118ae65c8, thisValue={m_ptr = 0x118e73440}, context=0x1073cc668) at Source/WebCore/bindings/js/ScheduledAction.cpp:106
#18 0x0000000101b18dfc in WebCore::ScheduledAction::execute (this=0x11a4723d0, document=0x1073cc600) at Source/WebCore/bindings/js/ScheduledAction.cpp:128
#19 0x0000000101b18ed0 in WebCore::ScheduledAction::execute (this=0x11a4723d0, context=0x1073cc668) at Source/WebCore/bindings/js/ScheduledAction.cpp:76
#20 0x00000001012ac1d5 in WebCore::DOMTimer::fired (this=0x11d180c40) at Source/WebCore/page/DOMTimer.cpp:130
#21 0x0000000101cae3ea in WebCore::ThreadTimers::sharedTimerFiredInternal (this=0x105e6ac50) at Source/WebCore/platform/ThreadTimers.cpp:112
#22 0x0000000101cae605 in WebCore::ThreadTimers::sharedTimerFired () at Source/WebCore/platform/ThreadTimers.cpp:90
#23 0x0000000101b7fd08 in WebCore::timerFired () at Source/WebCore/platform/mac/SharedTimerMac.mm:166
#24 0x00007fff870c9678 in __CFRunLoopRun ()
#25 0x00007fff870c784f in CFRunLoopRunSpecific ()
#26 0x00007fff86e45a18 in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] ()
#27 0x00000001000116d1 in runTest (testPathOrURL=@0x7fff5fbff2e0) at Tools/DumpRenderTree/mac/DumpRenderTree.mm:1127
#28 0x0000000100011bc8 in runTestingServerLoop () at Tools/DumpRenderTree/mac/DumpRenderTree.mm:615
#29 0x0000000100011fe2 in dumpRenderTree (argc=2, argv=0x7fff5fbffbf8) at Tools/DumpRenderTree/mac/DumpRenderTree.mm:671
#30 0x0000000100012204 in main (argc=2, argv=0x7fff5fbffbf8) at Tools/DumpRenderTree/mac/DumpRenderTree.mm:712


inspector/console/console-api-on-call-frame.html -> crashed
...................
inspector/debugger .
inspector/debugger/debug-inlined-scripts.html -> crashed
...
inspector/debugger/debugger-cyclic-ref.html -> crashed
.
inspector/debugger/debugger-eval-on-call-frame.html -> crashed
.
inspector/debugger/debugger-eval-while-paused.html -> crashed
.
inspector/debugger/debugger-expand-scope.html -> crashed
.
inspector/debugger/debugger-no-nested-pause.html -> crashed
.
inspector/debugger/debugger-pause-in-eval-script.html -> crashed
.
inspector/debugger/debugger-pause-on-breakpoint.html -> crashed
.
inspector/debugger/debugger-pause-on-debugger-statement.html -> crashed
.
inspector/debugger/debugger-pause-on-exception.html -> crashed
.
inspector/debugger/debugger-proto-property.html -> crashed
..
inspector/debugger/debugger-step-in.html -> crashed
.
inspector/debugger/debugger-step-out.html -> crashed
.
inspector/debugger/debugger-step-over.html -> crashed
.
inspector/debugger/debugger-suspend-active-dom-objects.html -> crashed
..

[Philippe Normand]

(In reply to comment #57)
> This change broke inspector debugger tests on the Leopard Debug Tests bot. 
> 

Pavel, see patch in bug 55941.

It fixes the inspector tests on GTK at least.

[Alice Liu]

This checkin seems to have caused bug 56073

[Pavel Feldman]

(In reply to comment #59)
> This checkin seems to have caused bug 56073

Given inspector breakage and now engadget.com breakage, do we want to roll it out? If Leopard Debug bots were alive at the time of the commit, this would have been rolled out immediately. So if I hear no objections, I'll roll it out in couple of hours and reopen the bug.

[Pavel Feldman]

Rolled out as r80718

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	D	Source/WebCore/manual-tests/database-callback-deferred.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/gtk/Skipped
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/dom/Document.cpp
	M	Source/WebCore/dom/Document.h
	M	Source/WebCore/loader/FrameLoader.cpp
	M	Source/WebCore/page/PageGroupLoadDeferrer.cpp
Committed r80718

[Yong Li]

These regressions also mean PageGroupLoadDeferrer is frequently used by WebKit2 / inspector, so the patch is really needed. Regressions should be fixed though

[Yong Li]

Now I got it. My patch moves the following lines to FrameLoader::setDefersLoading, then Page::setDefersLoading() runs them. If someone calls Page::setDefersLoading() directly, it will conflict with PageGroupLoadDeferrer.

                for (Frame* frame = otherPage->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
                    frame->document()->suspendScriptedAnimationControllerCallbacks();
                    frame->document()->suspendActiveDOMObjects(ActiveDOMObject::WillShowDialog);
                    frame->document()->scriptRunner()->suspend();
                    if (DocumentParser* parser = frame->document()->parser())
                        parser->suspendScheduledTasks();
                }

[Yong Li]

Created attachment 118230 [details]
Remove the part that causes conflict with other callers of Page::setDefersLoading().

[Early Warning System Bot]

Comment on attachment 118230 [details]
Remove the part that causes conflict with other callers of Page::setDefersLoading().

Attachment 118230 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10753211

[WebKit Review Bot]

Comment on attachment 118230 [details]
Remove the part that causes conflict with other callers of Page::setDefersLoading().

Attachment 118230 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10767138

[Yong Li]

Created attachment 118241 [details]
fix the && || warning

[WebKit Review Bot]

Comment on attachment 118241 [details]
fix the && || warning

Clearing flags on attachment: 118241

Committed r102278: <http://trac.webkit.org/changeset/102278>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Brady Eidson]

>If someone calls Page::setDefersLoading() directly, it will conflict with PageGroupLoadDeferrer.

It's unfortunate that a bit of exploration wasn't made to demonstrate that Page::setDefersLoading() has been used directly in WebKit API/SPI since the beginning of the WebKit open source project.

(See bug https://bugs.webkit.org/show_bug.cgi?id=84488)