Bug 49401 - Defer ScriptExecutionContext::Task's in Document when page loading is deferred
Summary: Defer ScriptExecutionContext::Task's in Document when page loading is deferred
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yong Li
URL:
Keywords:
Depends on:
Blocks: 73945
  Show dependency treegraph
 
Reported: 2010-11-11 12:42 PST by Yong Li
Modified: 2012-04-23 10:52 PDT (History)
15 users (show)

See Also:


Attachments
the patch (7.29 KB, patch)
2010-11-11 13:51 PST, Yong Li
no flags Details | Formatted Diff | Diff
updated (7.30 KB, patch)
2010-11-11 13:59 PST, Yong Li
no flags Details | Formatted Diff | Diff
fix style (7.30 KB, patch)
2010-11-11 14:01 PST, Yong Li
no flags Details | Formatted Diff | Diff
remove an unused variable in test case (7.26 KB, patch)
2010-11-19 07:17 PST, Yong Li
eric: review-
Details | Formatted Diff | Diff
try again (7.30 KB, patch)
2010-12-13 08:42 PST, Yong Li
no flags Details | Formatted Diff | Diff
finally found the warning that fails build (7.30 KB, patch)
2010-12-13 10:47 PST, Yong Li
darin: review-
Details | Formatted Diff | Diff
updated (8.45 KB, patch)
2010-12-13 14:07 PST, Yong Li
no flags Details | Formatted Diff | Diff
rerun builtbot (8.45 KB, patch)
2010-12-14 13:17 PST, Yong Li
no flags Details | Formatted Diff | Diff
updated (8.60 KB, patch)
2010-12-21 14:02 PST, Yong Li
commit-queue: commit-queue-
Details | Formatted Diff | Diff
fix merge error (7.30 KB, patch)
2011-01-04 12:55 PST, Yong Li
yong.li.webkit: commit-queue+
Details | Formatted Diff | Diff
the right one (9.08 KB, patch)
2011-01-04 12:56 PST, Yong Li
yong.li.webkit: commit-queue+
Details | Formatted Diff | Diff
try again (9.08 KB, patch)
2011-01-05 13:22 PST, Yong Li
commit-queue: commit-queue-
Details | Formatted Diff | Diff
ertry (9.08 KB, patch)
2011-01-06 07:20 PST, Yong Li
yong.li.webkit: commit-queue+
Details | Formatted Diff | Diff
Change the expected result of worker-close-more test (10.37 KB, patch)
2011-01-06 10:40 PST, Yong Li
no flags Details | Formatted Diff | Diff
Based on latest code (9.47 KB, patch)
2011-02-16 08:53 PST, Yong Li
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Let commit log test the patch (10.71 KB, patch)
2011-02-16 10:41 PST, Yong Li
yong.li.webkit: commit-queue-
Details | Formatted Diff | Diff
up-to-date (9.52 KB, patch)
2011-03-03 14:49 PST, Yong Li
yong.li.webkit: commit-queue-
Details | Formatted Diff | Diff
Try again (9.49 KB, patch)
2011-03-07 09:06 PST, Yong Li
no flags Details | Formatted Diff | Diff
Remove the part that causes conflict with other callers of Page::setDefersLoading(). (8.35 KB, patch)
2011-12-07 10:47 PST, Yong Li
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
fix the && || warning (8.36 KB, patch)
2011-12-07 11:30 PST, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2010-11-11 12:42:30 PST
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.
Comment 1 Yong Li 2010-11-11 13:51:35 PST
Created attachment 73654 [details]
the patch
Comment 2 Yong Li 2010-11-11 13:53:00 PST
Comment on attachment 73654 [details]
the patch

test case should be rewritten
Comment 3 WebKit Review Bot 2010-11-11 13:55:53 PST
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.
Comment 4 Yong Li 2010-11-11 13:59:36 PST
Created attachment 73657 [details]
updated
Comment 5 Yong Li 2010-11-11 14:01:33 PST
Created attachment 73658 [details]
fix style
Comment 6 WebKit Review Bot 2010-11-11 14:02:55 PST
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.
Comment 7 Eric Seidel (no email) 2010-11-11 15:39:40 PST
Attachment 73654 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5702008
Comment 8 Eric Seidel (no email) 2010-11-11 16:54:44 PST
Attachment 73658 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/5670006
Comment 9 Eric Seidel (no email) 2010-11-11 17:35:00 PST
Attachment 73658 [details] did not build on mac:
Build output: http://queues.webkit.org/results/5771004
Comment 10 Yong Li 2010-11-19 07:17:17 PST
Created attachment 74385 [details]
remove an unused variable in test case
Comment 11 Eric Seidel (no email) 2010-11-19 07:47:35 PST
Attachment 74385 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6229071
Comment 12 Eric Seidel (no email) 2010-11-19 09:02:45 PST
Attachment 74385 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6197085
Comment 13 Eric Seidel (no email) 2010-11-19 10:19:34 PST
Attachment 74385 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6204053
Comment 14 Eric Seidel (no email) 2010-12-10 22:21:12 PST
Comment on attachment 74385 [details]
remove an unused variable in test case

Would break mac.
Comment 15 Yong Li 2010-12-13 08:42:39 PST
Created attachment 76389 [details]
try again
Comment 16 WebKit Review Bot 2010-12-13 09:20:12 PST
Attachment 76389 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6984090
Comment 17 Eric Seidel (no email) 2010-12-13 10:36:59 PST
Attachment 76389 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6891121
Comment 18 Eric Seidel (no email) 2010-12-13 10:39:48 PST
Attachment 76389 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7042024
Comment 19 Yong Li 2010-12-13 10:47:19 PST
Created attachment 76407 [details]
finally found the warning that fails build
Comment 20 Darin Adler 2010-12-13 10:58:23 PST
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.
Comment 21 Yong Li 2010-12-13 11:54:17 PST
(In reply to comment #20)

Many cool ideas. Working on another patch.

suspendActiveDOMObjects cannot be private because it is from parent class: ScriptExecutionContext
Comment 22 Yong Li 2010-12-13 14:07:26 PST
Created attachment 76435 [details]
updated

cannot use Deque<OwnPtr<T> >
Comment 23 Eric Seidel (no email) 2010-12-13 15:38:25 PST
Attachment 76435 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/6993098
Comment 24 Yong Li 2010-12-14 08:20:08 PST
(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?
Comment 25 Eric Seidel (no email) 2010-12-14 11:56:08 PST
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.
Comment 26 Yong Li 2010-12-14 13:17:00 PST
Created attachment 76562 [details]
rerun builtbot
Comment 27 Darin Adler 2010-12-21 11:51:30 PST
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.
Comment 28 Yong Li 2010-12-21 13:19:01 PST
(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, ...
Comment 29 Yong Li 2010-12-21 14:02:14 PST
Created attachment 77150 [details]
updated
Comment 30 WebKit Commit Bot 2010-12-21 17:42:52 PST
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
Comment 31 Yong Li 2011-01-04 12:55:13 PST
Created attachment 77919 [details]
fix merge error
Comment 32 Yong Li 2011-01-04 12:56:15 PST
Created attachment 77920 [details]
the right one
Comment 33 WebKit Commit Bot 2011-01-04 23:34:57 PST
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
Comment 34 Yong Li 2011-01-05 13:22:04 PST
Created attachment 78032 [details]
try again
Comment 35 WebKit Commit Bot 2011-01-05 18:23:59 PST
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
Comment 36 Eric Seidel (no email) 2011-01-05 18:28:59 PST
Comment on attachment 78032 [details]
try again

It's possible that simply flaked twice.
Comment 37 WebKit Commit Bot 2011-01-05 21:04:51 PST
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
Comment 38 Yong Li 2011-01-06 07:20:38 PST
Created attachment 78117 [details]
ertry
Comment 39 Yong Li 2011-01-06 09:17:26 PST
(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?
Comment 40 Yong Li 2011-01-06 10:40:25 PST
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.
Comment 41 Eric Seidel (no email) 2011-01-31 16:07:26 PST
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.
Comment 42 Yong Li 2011-02-16 06:50:55 PST
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
Comment 43 Yong Li 2011-02-16 08:53:02 PST
Created attachment 82647 [details]
Based on latest code
Comment 44 Yong Li 2011-02-16 10:26:12 PST
Comment on attachment 82647 [details]
Based on latest code

still fail the test
Comment 45 Yong Li 2011-02-16 10:41:36 PST
Created attachment 82660 [details]
Let commit log test the patch
Comment 46 Yong Li 2011-02-16 12:35:14 PST
Comment on attachment 82660 [details]
Let commit log test the patch

Darnn. still fail the test
Comment 47 WebKit Commit Bot 2011-03-03 14:02:08 PST
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
Comment 48 Yong Li 2011-03-03 14:49:19 PST
Created attachment 84630 [details]
up-to-date
Comment 49 Yong Li 2011-03-07 09:06:48 PST
Created attachment 84950 [details]
Try again
Comment 50 WebKit Commit Bot 2011-03-07 10:55:14 PST
Comment on attachment 84950 [details]
Try again

Clearing flags on attachment: 84950

Committed r80478: <http://trac.webkit.org/changeset/80478>
Comment 51 WebKit Commit Bot 2011-03-07 10:55:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 52 WebKit Review Bot 2011-03-07 11:10:55 PST
http://trac.webkit.org/changeset/80478 might have broken Qt Linux Release
Comment 53 Csaba Osztrogonác 2011-03-07 11:20:39 PST
(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
Comment 54 Ryosuke Niwa 2011-03-07 20:31:58 PST
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
Comment 55 Martin Robinson 2011-03-07 21:39:58 PST
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
Comment 56 Philippe Normand 2011-03-08 01:37:15 PST
Is someone looking at the inspector crashes caused by this patch?
Comment 57 Pavel Feldman 2011-03-09 07:10:35 PST
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
..
Comment 58 Philippe Normand 2011-03-09 07:17:35 PST
(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.
Comment 59 Alice Liu 2011-03-09 22:20:28 PST
This checkin seems to have caused bug 56073
Comment 60 Pavel Feldman 2011-03-09 22:33:54 PST
(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.
Comment 61 Pavel Feldman 2011-03-10 07:18:38 PST
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
Comment 62 Yong Li 2011-03-10 14:03:33 PST
These regressions also mean PageGroupLoadDeferrer is frequently used by WebKit2 / inspector, so the patch is really needed. Regressions should be fixed though
Comment 63 Yong Li 2011-12-07 10:18:19 PST
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();
                }
Comment 64 Yong Li 2011-12-07 10:47:28 PST
Created attachment 118230 [details]
Remove the part that causes conflict with other callers of Page::setDefersLoading().
Comment 65 Early Warning System Bot 2011-12-07 11:03:07 PST
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
Comment 66 WebKit Review Bot 2011-12-07 11:12:23 PST
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
Comment 67 Yong Li 2011-12-07 11:30:02 PST
Created attachment 118241 [details]
fix the && || warning
Comment 68 WebKit Review Bot 2011-12-07 15:41:36 PST
Comment on attachment 118241 [details]
fix the && || warning

Clearing flags on attachment: 118241

Committed r102278: <http://trac.webkit.org/changeset/102278>
Comment 69 WebKit Review Bot 2011-12-07 15:41:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 70 Brady Eidson 2012-04-23 10:52:57 PDT
>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)