WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 108984
Load event fires too early with threaded HTML parser
https://bugs.webkit.org/show_bug.cgi?id=108984
Summary
Load event fires too early with threaded HTML parser
Adam Barth
Reported
2013-02-05 14:52:35 PST
Load event fires too early with threaded HTML parser
Attachments
work in progress
(9.99 KB, patch)
2013-02-05 14:53 PST
,
Adam Barth
no flags
Details
Formatted Diff
Diff
passes foo.html test case
(8.72 KB, patch)
2013-02-07 13:31 PST
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch
(9.62 KB, patch)
2013-02-07 14:23 PST
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch for landing
(9.06 KB, patch)
2013-02-07 16:44 PST
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Patch for landing
(10.75 KB, patch)
2013-02-08 22:55 PST
,
Adam Barth
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
2013-02-05 14:53:06 PST
Created
attachment 186712
[details]
work in progress
Eric Seidel (no email)
Comment 2
2013-02-05 15:00:58 PST
Comment on
attachment 186712
[details]
work in progress I'm not sure if it makes sense for the parser to extend the document's lifetime or not. Another approch is to put this off onto its own separate ref-counted object. But it seems reasonable to me. We could just make processingData() not be public, and instead have a new Document::parsingInProgress() or something which checks both of these on Document. I think that woudl be better abstraction than DocumetnLoader knowing about DocumetnParser.
Adam Barth
Comment 3
2013-02-05 15:40:12 PST
> I'm not sure if it makes sense for the parser to extend the document's lifetime or not. Another approch is to put this off onto its own separate ref-counted object.
I see. The document loader would find the object from the document in isLoadingInAPISense. Yeah, that seems better than refing the document. By the way, this patch doesn't work yet. It seems to delay the load event forever in this test case: <script> if (window.testRunner) { testRunner.dumpAsText(); } window.onload = function() { alert(42); }; </script> <iframe onload="alert(0);document.write('<p>PASS</p>');alert(2);document.close();alert(3);"></iframe> FAIL
Adam Barth
Comment 4
2013-02-07 13:31:06 PST
Created
attachment 187167
[details]
passes foo.html test case
Eric Seidel (no email)
Comment 5
2013-02-07 13:35:47 PST
Comment on
attachment 187167
[details]
passes foo.html test case View in context:
https://bugs.webkit.org/attachment.cgi?id=187167&action=review
> Source/WebCore/dom/Document.cpp:5781 > + if (!frame()->loader()->stateMachine()->creatingInitialEmptyDocument())
WAT?
> Source/WebCore/html/parser/HTMLParserScheduler.cpp:65 > + : document(doc)
m_document? I'm sure this is private. It's not clear to me if PumpSession should really hold a Document*.
> Source/WebCore/loader/DocumentLoader.cpp:485 > + if (doc->parserSessionNestingLevel() > 0) > + return true; > if (DocumentParser* parser = doc->parser()) > if (parser->processingData()) > return true;
I would combine these all into one if, moving the processingData() function onto Document. Not surew ht the name is. isParsing()? parserIsActive?
Adam Barth
Comment 6
2013-02-07 14:23:01 PST
Created
attachment 187176
[details]
Patch
Eric Seidel (no email)
Comment 7
2013-02-07 14:27:25 PST
Comment on
attachment 187176
[details]
Patch This looks fantastic.
WebKit Review Bot
Comment 8
2013-02-07 16:38:41 PST
Comment on
attachment 187176
[details]
Patch Rejecting
attachment 187176
[details]
from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 187176, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: at 305. Hunk #2 succeeded at 365 (offset 9 lines). 1 out of 2 hunks FAILED -- saving rejects to file Source/WebCore/html/parser/HTMLDocumentParser.cpp.rej patching file Source/WebCore/html/parser/HTMLParserScheduler.cpp patching file Source/WebCore/html/parser/HTMLParserScheduler.h patching file Source/WebCore/loader/DocumentLoader.cpp Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output:
http://queues.webkit.org/results/16429390
Adam Barth
Comment 9
2013-02-07 16:44:25 PST
Created
attachment 187198
[details]
Patch for landing
WebKit Review Bot
Comment 10
2013-02-07 19:28:46 PST
Comment on
attachment 187198
[details]
Patch for landing Rejecting
attachment 187198
[details]
from commit-queue. New failing tests: perf/nested-combined-selectors.html perf/accessibility-title-ui-element.html perf/document-contains.html fast/parser/empty-text-resource.html Full output:
http://queues.webkit.org/results/16426462
Adam Barth
Comment 11
2013-02-08 22:55:35 PST
Created
attachment 187418
[details]
Patch for landing
WebKit Review Bot
Comment 12
2013-02-09 11:35:46 PST
Comment on
attachment 187418
[details]
Patch for landing Clearing flags on attachment: 187418 Committed
r142378
: <
http://trac.webkit.org/changeset/142378
>
WebKit Review Bot
Comment 13
2013-02-09 11:35:50 PST
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 14
2013-02-09 18:12:26 PST
This broke the Apple Windows build:
http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/62203
7>WebCore.lib(DocumentLoader.obj) : error LNK2001: unresolved external symbol "public: bool __thiscall WebCore::Document::hasActiveParser(void)" (?hasActiveParser@Document@WebCore@@QAE_NXZ) 7>WebCore.lib(HTMLParserScheduler.obj) : error LNK2001: unresolved external symbol "public: void __thiscall WebCore::Document::decrementActiveParserCount(void)" (?decrementActiveParserCount@Document@WebCore@@QAEXXZ)
Eric Seidel (no email)
Comment 15
2013-02-09 18:32:33 PST
ah. Export files. :)
Adam Barth
Comment 16
2013-02-09 21:02:48 PST
Sorry for breaking the build. We probably just need to add ?decrementActiveParserCount@Document@WebCore@@QAEXXZ to the export file. Unfortunately, I don't have a working copy right now. Please feel free to roll out my patch if that's easier.
Adam Barth
Comment 17
2013-02-09 22:29:28 PST
It looks like it's only Apple Win Debug (Build). Investigating.
Adam Barth
Comment 18
2013-02-09 22:34:57 PST
I think it's a problem with the builder. If you look at <
http://build.webkit.org/builders/Apple%20Win%20Debug%20%28Build%29/builds/62203/steps/compile-webkit/logs/stdio
>, which is the first cycle with my patch, you'll see that it didn't recompile Document.cpp for whatever reason. Those functions are defined in Document.cpp and not used outside WebCore. They shouldn't need to be exported. (Note that the release build also works fine.) We should try a clobber build.
Adam Barth
Comment 19
2013-02-10 07:49:16 PST
The bot is now healed after another patch touched Platform.h:
http://build.webkit.org/waterfall?show=Apple%20Win%20Debug%20(Build
) I suspect there's some sort of bad dependency in the Windows build.
Kentaro Hara
Comment 20
2013-02-10 17:32:18 PST
It looks like the patch broke fast/frames/seamless/seamless-inherited-origin.html For now, I marked the test as FAIL. Please take a look:
https://bugs.webkit.org/show_bug.cgi?id=109391
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug