Bug 108984 - Load event fires too early with threaded HTML parser
Summary: Load event fires too early with threaded HTML parser
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 109465
Blocks: 106127
  Show dependency treegraph
 
Reported: 2013-02-05 14:52 PST by Adam Barth
Modified: 2013-02-11 11:24 PST (History)
7 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2013-02-05 14:52:35 PST
Load event fires too early with threaded HTML parser
Comment 1 Adam Barth 2013-02-05 14:53:06 PST
Created attachment 186712 [details]
work in progress
Comment 2 Eric Seidel (no email) 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.
Comment 3 Adam Barth 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
Comment 4 Adam Barth 2013-02-07 13:31:06 PST
Created attachment 187167 [details]
passes foo.html test case
Comment 5 Eric Seidel (no email) 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?
Comment 6 Adam Barth 2013-02-07 14:23:01 PST
Created attachment 187176 [details]
Patch
Comment 7 Eric Seidel (no email) 2013-02-07 14:27:25 PST
Comment on attachment 187176 [details]
Patch

This looks fantastic.
Comment 8 WebKit Review Bot 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
Comment 9 Adam Barth 2013-02-07 16:44:25 PST
Created attachment 187198 [details]
Patch for landing
Comment 10 WebKit Review Bot 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
Comment 11 Adam Barth 2013-02-08 22:55:35 PST
Created attachment 187418 [details]
Patch for landing
Comment 12 WebKit Review Bot 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>
Comment 13 WebKit Review Bot 2013-02-09 11:35:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Simon Fraser (smfr) 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)
Comment 15 Eric Seidel (no email) 2013-02-09 18:32:33 PST
ah. Export files. :)
Comment 16 Adam Barth 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.
Comment 17 Adam Barth 2013-02-09 22:29:28 PST
It looks like it's only Apple Win Debug (Build).  Investigating.
Comment 18 Adam Barth 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.
Comment 19 Adam Barth 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.
Comment 20 Kentaro Hara 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