Load event fires too early with threaded HTML parser

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Load event fires too early with threaded HTML parser

[Adam Barth]

Created attachment 186712 [details]
work in progress

[Eric Seidel (no email)]

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]

> 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]

Created attachment 187167 [details]
passes foo.html test case

[Eric Seidel (no email)]

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]

Created attachment 187176 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 187176 [details]
Patch

This looks fantastic.

[WebKit Review Bot]

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]

Created attachment 187198 [details]
Patch for landing

[WebKit Review Bot]

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]

Created attachment 187418 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187418 [details]
Patch for landing

Clearing flags on attachment: 187418

Committed r142378: <http://trac.webkit.org/changeset/142378>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

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)]

ah. Export files. :)

[Adam Barth]

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]

It looks like it's only Apple Win Debug (Build).  Investigating.

[Adam Barth]

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]

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]

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