Bug 50489 – Move DocumentWriter to DocumentLoader

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

Currently, DocumentWriter is owned by FrameLoader, but it's logically per-load functionality, so it should probably belong to DocumentLoader.

Patch coming shortly.

[Nate Chapin]

Created attachment 75549 [details]
Attempt #1

[Adam Barth]

Comment on attachment 75549 [details]
Attempt #1

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

This patch is great.  Thanks for doing this!  Some comments below to think about before landing.

> WebKit/wx/WebKitSupport/FrameLoaderClientWx.cpp:437
> -            FrameLoader* fl = loader->frameLoader();
> -            fl->writer()->setEncoding(m_response.textEncodingName(), false);
> +            loader->writer()->setEncoding(m_response.textEncodingName(), false);

:)

> WebKit/wince/WebCoreSupport/FrameLoaderClientWinCE.cpp:453
> -        FrameLoader* loader = documentLoader->frameLoader();
> -        loader->writer()->setEncoding(m_response.textEncodingName(), false);
> +        documentLoader->writer()->setEncoding(m_response.textEncodingName(), false);

:)

> WebCore/dom/Document.cpp:432
> +        m_documentLoader->writer()->setFrame(frame);

Hum...  This is a small sadness.

> WebCore/dom/Document.cpp:4449
> -        DocumentLoader* documentLoader = m_frame->loader()->documentLoader();
> -        if (documentLoader && documentLoader->substituteData().isValid())
> +        if (m_documentLoader->substituteData().isValid())

I really like that Document has a pointer to DocumentLoader.  Going through the Frame / FrameLoader to get the DocumentLoader is just asking for trouble.

> WebCore/dom/Document.cpp:4528
>      f->loader()->setURL(url);

I dream that someday you'll get your patch to remove this working.  :)

> WebCore/html/HTMLLinkElement.cpp:219
> -        if (charset.isEmpty() && document()->frame())
> -            charset = document()->frame()->loader()->writer()->encoding();
> +        if (charset.isEmpty())
> +            charset = document()->loader()->writer()->encoding();

Nice.

> WebCore/loader/FrameLoader.cpp:235
> -    writer()->begin(KURL(), false);
> -    writer()->end();
> +    m_documentLoader->writer()->begin(KURL(), false);
> +    m_documentLoader->writer()->end();

This doesn't need to be m_provisionalDocumentLoader ?  I guess nothing would work if it was wrong, so it must be right.

> WebCore/loader/FrameLoader.cpp:595
>      m_frame->setDocument(0);
> -    writer()->clear();
> +    if (activeDocumentLoader())
> +        activeDocumentLoader()->writer()->clear();

Does it make sense to get the document loader from the document here instead?

> WebCore/loader/FrameLoader.cpp:2676
>      Settings* settings = m_frame->settings();
> -    request.setResponseContentDispositionEncodingFallbackArray("UTF-8", writer()->deprecatedFrameEncoding(), settings ? settings->defaultTextEncodingName() : String());
> +    request.setResponseContentDispositionEncodingFallbackArray("UTF-8", activeDocumentLoader()->writer()->deprecatedFrameEncoding(), settings ? settings->defaultTextEncodingName() : String());

Sigh.  This code is so evil, but not really your fault.

> WebCore/loader/DocumentLoader.cpp:378
> +    m_writer.setFrame(frame);

Why do we need to call setFrame in two places?  I would have expected this one to be sufficient.

By the way, another approach is to give DocumentWriter a back-pointer to DocumentLoader, which then has this back-pointer to Frame.  I'm not sure that really buys you anything over the approach you have here though.

[Nate Chapin]

(In reply to comment #2)
> > WebCore/dom/Document.cpp:432
> > +        m_documentLoader->writer()->setFrame(frame);
> 
> Hum...  This is a small sadness.
> 

Indeed.  I'll double-check that it's really necessary, but I'm afraid it is.

> > WebCore/dom/Document.cpp:4449
> > -        DocumentLoader* documentLoader = m_frame->loader()->documentLoader();
> > -        if (documentLoader && documentLoader->substituteData().isValid())
> > +        if (m_documentLoader->substituteData().isValid())
> 
> I really like that Document has a pointer to DocumentLoader.  Going through the Frame / FrameLoader to get the DocumentLoader is just asking for trouble.

:)

> 
> > WebCore/dom/Document.cpp:4528
> >      f->loader()->setURL(url);
> 
> I dream that someday you'll get your patch to remove this working.  :)

I should take another stab at that, but I cringe every time I think about it...


> 
> > WebCore/loader/FrameLoader.cpp:235
> > -    writer()->begin(KURL(), false);
> > -    writer()->end();
> > +    m_documentLoader->writer()->begin(KURL(), false);
> > +    m_documentLoader->writer()->end();
> 
> This doesn't need to be m_provisionalDocumentLoader ?  I guess nothing would work if it was wrong, so it must be right.

I think about suing activeDocumentLoader() here, but decided that it would be clearer if I didn't.  The quirkiness here is that DocumentLoader::finishedLoading() causes us to commit, so the previous line guarantees we'll be committed at this point.

> 
> > WebCore/loader/FrameLoader.cpp:595
> >      m_frame->setDocument(0);
> > -    writer()->clear();
> > +    if (activeDocumentLoader())
> > +        activeDocumentLoader()->writer()->clear();
> 
> Does it make sense to get the document loader from the document here instead?

Probably, I'll try it out.

> > WebCore/loader/DocumentLoader.cpp:378
> > +    m_writer.setFrame(frame);
> 
> Why do we need to call setFrame in two places?  I would have expected this one to be sufficient.

I think one of these setFrame() calls is vestigal, but I need to make sure.

[Nate Chapin]

http://trac.webkit.org/changeset/73392

[Martin Robinson]

This change has been causing some crashes on the GTK+ bots with the following tests:

fast/frames/iframe-onload-remove-self-no-crash.html
fast/loader/frame-creation-removal.html

Here's the stack trace from one of these crashes:

Thread 1 (Thread 25202):
#0  0x00002b8c57da23bc in WebCore::DocumentWriter::end (this=0x100) at ../../WebCore/loader/DocumentWriter.cpp:206
#1  0x00002b8c57da5a67 in WebCore::FrameLoader::init (this=0x527fc28) at ../../WebCore/loader/FrameLoader.cpp:235
#2  0x00002b8c58175c46 in WebCore::Frame::init (this=0x527fbd0) at ../../WebCore/page/Frame.h:253
#3  0x00002b8c582f0ad6 in WebKit::FrameLoaderClient::createFrame (this=0x1a2ce00, url=..., name=..., ownerElement=0x532dee0, referrer=..., allowsScrolling=true, marginWidth=-1, marginHeight=-1) at ../../WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:590
#4  0x00002b8c57df674f in WebCore::SubframeLoader::loadSubframe (this=0x1a33a28, ownerElement=0x532dee0, url=..., name=..., referrer=...) at ../../WebCore/loader/SubframeLoader.cpp:266
#5  0x00002b8c57df653d in WebCore::SubframeLoader::loadOrRedirectSubframe (this=0x1a33a28, ownerElement=0x532dee0, url=..., frameName=..., lockHistory=true, lockBackForwardList=true) at ../../WebCore/loader/SubframeLoader.cpp:244
#6  0x00002b8c57df5b27 in WebCore::SubframeLoader::requestFrame (this=0x1a33a28, ownerElement=0x532dee0, urlString=..., frameName=..., lockHistory=true, lockBackForwardList=true) at ../../WebCore/loader/SubframeLoader.cpp:82
#7  0x00002b8c57c5417f in WebCore::HTMLFrameElementBase::openURL (this=0x532dee0, lockHistory=true, lockBackForwardList=true) at ../../WebCore/html/HTMLFrameElementBase.cpp:105
#8  0x00002b8c57c547a6 in WebCore::HTMLFrameElementBase::setNameAndOpenURL (this=0x532dee0) at ../../WebCore/html/HTMLFrameElementBase.cpp:156
#9  0x00002b8c57c548e7 in WebCore::HTMLFrameElementBase::insertedIntoDocument (this=0x532dee0) at ../../WebCore/html/HTMLFrameElementBase.cpp:190
#10 0x00002b8c57c58f73 in WebCore::HTMLIFrameElement::insertedIntoDocument (this=0x532dee0) at ../../WebCore/html/HTMLIFrameElement.cpp:149
#11 0x00002b8c57a9cb42 in WebCore::notifyChildInserted (child=0x532dee0) at ../../WebCore/dom/ContainerNode.cpp:1015
#12 0x00002b8c57a9b1e9 in WebCore::ContainerNode::appendChild (this=0x5172310, newChild=..., ec=@0x7ffff9f2d61c, shouldLazyAttach=true) at ../../WebCore/dom/ContainerNode.cpp:604
#13 0x00002b8c57b10724 in WebCore::Node::appendChild (this=0x5172310, newChild=..., ec=@0x7ffff9f2d61c, shouldLazyAttach=true) at ../../WebCore/dom/Node.cpp:569
#14 0x00002b8c57934bed in WebCore::JSNode::appendChild (this=0x2b8cf0290440, exec=0x2b8cefe430d8) at ../../WebCore/bindings/js/JSNodeCustom.cpp:108
#15 0x00002b8c5847f921 in WebCore::jsNodePrototypeFunctionAppendChild (exec=0x2b8cefe430d8) at DerivedSources/WebCore/JSNode.cpp:484
#16 0x00002b8c6fe331b8 in ?? ()
#17 0x00007ffff9f2d740 in ?? ()
#18 0x00002b8c6ff0b1d9 in ?? ()
#19 0x00007ffff9f2d6c0 in ?? ()
#20 0x00002b8cf0290440 in ?? ()
#21 0x0000000004f20078 in ?? ()
#22 0x00002b8cf028d340 in ?? ()
#23 0x00007ffff9f2d6f0 in ?? ()
#24 0x00002b8c5790bc13 in JSC::Register::Register (this=0x50dfe70) at ../../JavaScriptCore/interpreter/Register.h:106
#25 0x00002b8c586db9bd in JSC::JITCode::execute (this=0x50d4c58, registerFile=0x1ae11f8, callFrame=0x2b8cefe43040, globalData=0x1ade980) at ../../JavaScriptCore/jit/JITCode.h:77
#26 0x00002b8c586d88f3 in JSC::Interpreter::executeCall (this=0x1ae11e0, callFrame=0x5065ff8, function=0x2b8cf028ffc0, callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../JavaScriptCore/interpreter/Interpreter.cpp:849
#27 0x00002b8c58767a1f in JSC::call (exec=0x5065ff8, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../JavaScriptCore/runtime/CallData.cpp:38
#28 0x00002b8c578e8619 in WebCore::JSMainThreadExecState::call (exec=0x5065ff8, functionObject=..., callType=JSC::CallTypeJS, callData=..., thisValue=..., args=...) at ../../WebCore/bindings/js/JSMainThreadExecState.h:48
#29 0x00002b8c57922794 in WebCore::JSEventListener::handleEvent (this=0x50fef00, scriptExecutionContext=0x50893e8, event=0x50d4bf0) at ../../WebCore/bindings/js/JSEventListener.cpp:124
#30 0x00002b8c57b0136d in WebCore::EventTarget::fireEventListeners (this=0x4d62850, event=0x50d4bf0, d=0x4d62930, entry=...) at ../../WebCore/dom/EventTarget.cpp:342
#31 0x00002b8c57b011fc in WebCore::EventTarget::fireEventListeners (this=0x4d62850, event=0x50d4bf0) at ../../WebCore/dom/EventTarget.cpp:311
#32 0x00002b8c57e35564 in WebCore::DOMWindow::dispatchEvent (this=0x4d62850, prpEvent=..., prpTarget=...) at ../../WebCore/page/DOMWindow.cpp:1549
#33 0x00002b8c57e3566e in WebCore::DOMWindow::dispatchTimedEvent (this=0x4d62850, event=..., target=0x5089380, startTime=0x50dfe68, endTime=0x50dfe70) at ../../WebCore/page/DOMWindow.cpp:1561
#34 0x00002b8c57e351b3 in WebCore::DOMWindow::dispatchLoadEvent (this=0x4d62850) at ../../WebCore/page/DOMWindow.cpp:1515
#35 0x00002b8c57ab9abd in WebCore::Document::dispatchWindowLoadEvent (this=0x5089380) at ../../WebCore/dom/Document.cpp:3461
#36 0x00002b8c57ab4aff in WebCore::Document::implicitClose (this=0x5089380) at ../../WebCore/dom/Document.cpp:2110
#37 0x00002b8c57da8a8d in WebCore::FrameLoader::checkCallImplicitClose (this=0x5280778) at ../../WebCore/loader/FrameLoader.cpp:901
#38 0x00002b8c57da8860 in WebCore::FrameLoader::checkCompleted (this=0x5280778) at ../../WebCore/loader/FrameLoader.cpp:849
#39 0x00002b8c57da85df in WebCore::FrameLoader::finishedParsing (this=0x5280778) at ../../WebCore/loader/FrameLoader.cpp:783
#40 0x00002b8c57abccd0 in WebCore::Document::finishedParsing (this=0x5089380) at ../../WebCore/dom/Document.cpp:4227
#41 0x00002b8c57cc8a85 in WebCore::HTMLTreeBuilder::finished (this=0x5281c20) at ../../WebCore/html/parser/HTMLTreeBuilder.cpp:2794
#42 0x00002b8c57ca0f94 in WebCore::HTMLDocumentParser::end (this=0x4f3c520) at ../../WebCore/html/parser/HTMLDocumentParser.cpp:323
#43 0x00002b8c57ca1087 in WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd (this=0x4f3c520) at ../../WebCore/html/parser/HTMLDocumentParser.cpp:332
#44 0x00002b8c57ca0578 in WebCore::HTMLDocumentParser::prepareToStopParsing (this=0x4f3c520) at ../../WebCore/html/parser/HTMLDocumentParser.cpp:150
#45 0x00002b8c57ca10cc in WebCore::HTMLDocumentParser::attemptToEnd (this=0x4f3c520) at ../../WebCore/html/parser/HTMLDocumentParser.cpp:344
#46 0x00002b8c57ca1185 in WebCore::HTMLDocumentParser::finish (this=0x4f3c520) at ../../WebCore/html/parser/HTMLDocumentParser.cpp:372
#47 0x00002b8c57ab50fa in WebCore::Document::finishParsing (this=0x5089380) at ../../WebCore/dom/Document.cpp:2261
#48 0x00002b8c57da2485 in WebCore::DocumentWriter::endIfNotLoadingMainResource (this=0x50df680) at ../../WebCore/loader/DocumentWriter.cpp:222
#49 0x00002b8c57da23db in WebCore::DocumentWriter::end (this=0x50df680) at ../../WebCore/loader/DocumentWriter.cpp:207
#50 0x00002b8c57d97167 in WebCore::DocumentLoader::finishedLoading (this=0x50df580) at ../../WebCore/loader/DocumentLoader.cpp:281
#51 0x00002b8c57daec27 in WebCore::FrameLoader::finishedLoading (this=0x5280778) at ../../WebCore/loader/FrameLoader.cpp:2165
#52 0x00002b8c57de2f51 in WebCore::MainResourceLoader::didFinishLoading (this=0x4e2efc0, finishTime=0) at ../../WebCore/loader/MainResourceLoader.cpp:457
#53 0x00002b8c57deee57 in WebCore::ResourceLoader::didFinishLoading (this=0x4e2efc0, finishTime=0) at ../../WebCore/loader/ResourceLoader.cpp:435
#54 0x00002b8c582c4269 in WebCore::closeCallback (source=0x2b8c680160c0, res=0x2b8c68016240) at ../../WebCore/platform/network/soup/ResourceHandleSoup.cpp:810
#55 0x00002b8c5b673620 in async_ready_close_callback_wrapper (source_object=0x2b8c680160c0, res=0x2b8c68016240, user_data=0x0) at /tmp/buildd/glib2.0-2.24.1/gio/ginputstream.c:485
#56 0x00002b8c5b681ac8 in complete_in_idle_cb_for_thread (_data=<value optimized out>) at /tmp/buildd/glib2.0-2.24.1/gio/gsimpleasyncresult.c:653
#57 0x00002b8c5c1766c2 in g_main_dispatch (context=0xffff000000000002) at /tmp/buildd/glib2.0-2.24.1/glib/gmain.c:1960
#58 IA__g_main_context_dispatch (context=0xffff000000000002) at /tmp/buildd/glib2.0-2.24.1/glib/gmain.c:2513
#59 0x00002b8c5c17a538 in g_main_context_iterate (context=0x1977f70, block=<value optimized out>, dispatch=<value optimized out>, self=<value optimized out>) at /tmp/buildd/glib2.0-2.24.1/glib/gmain.c:2591
#60 0x00002b8c5c17aa45 in IA__g_main_loop_run (loop=0x501cb70) at /tmp/buildd/glib2.0-2.24.1/glib/gmain.c:2799
#61 0x00002b8c5a17f657 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#62 0x000000000041a6d8 in runTest (testPathOrURL=...) at ../../WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:648
#63 0x0000000000419dad in runTestingServerLoop () at ../../WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:462
#64 0x000000000041bcae in main (argc=2, argv=0x7ffff9f2ef78) at ../../WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:1089

[Martin Robinson]

Committed r73436: <http://trac.webkit.org/changeset/73436>

[Nate Chapin]

Current theory for the crashes: My use of m_documentLoader->writer() instead of activeDocumentLoader()->writer() in FrameLoader::init() is only safe on certain platforms.

[Nate Chapin]

Created attachment 75922 [details]
Attempt to fix crashes in FrameLoader::init()

As described in the previous comment, the only difference between this and r73392 is the use of activeDocumentLoader() instead of m_documentLoader in FrameLoader::init().

[Nate Chapin]

Comment on attachment 75922 [details]
Attempt to fix crashes in FrameLoader::init()

Never mind, apparently the gtk crashes are still present with this patch.

[Eric Seidel (no email)]

Comment on attachment 75549 [details]
Attempt #1

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

[Nate Chapin]

Created attachment 77034 [details]
Attempt to fix crashes #2

Changes from the version that was committed and reverted:

FrameLoaderClientQt.cpp : Use m_frame->loader()->activeDocumentLoader() instead of m_frame->document()->loader().  I believe that this will be safer if we catch the loader in the middle of a navigation.

FrameLoaderClientGtk.h/cpp : Use an m_hasRepresentation boolean in the same way the qt and chromium FrameLoaderClients do.  This should keep us from blowing away our DocumentLoader in FrameLoader::init().

mrobinson and ossy, could I trouble you to verify this patch on your respective platforms at your leisure?

[Csaba Osztrogonác]

(In reply to comment #11)
> Created an attachment (id=77034) [details]
> Attempt to fix crashes #2
> 
> Changes from the version that was committed and reverted:
> 
> FrameLoaderClientQt.cpp : Use m_frame->loader()->activeDocumentLoader() instead of m_frame->document()->loader().  I believe that this will be safer if we catch the loader in the middle of a navigation.
> 
> FrameLoaderClientGtk.h/cpp : Use an m_hasRepresentation boolean in the same way the qt and chromium FrameLoaderClients do.  This should keep us from blowing away our DocumentLoader in FrameLoader::init().
> 
> mrobinson and ossy, could I trouble you to verify this patch on your respective platforms at your leisure?

I tried this patch with QtWebKit. The previously crashed API test passes, 
but unfortunately it caused a flakey layout test regression. Sometimes 
(every second or third execution of run-webkit-tests) there are 24 failing
tests with this error message "FAIL: Timed out waiting for notifyDone to be
called"

list of failing tests:
fast/tokenizer/flush-characters-in-document-write-evil.html
fast/tokenizer/flush-characters-in-document-write.html
fast/viewport/viewport-128.html
fast/workers/dedicated-worker-lifecycle.html
fast/workers/shared-worker-exception.html
fast/workers/shared-worker-lifecycle.html
fast/workers/shared-worker-name.html
fast/workers/shared-worker-script-error.html
fast/workers/stress-js-execution.html
fast/workers/termination-early.html
fast/workers/termination-with-port-messages.html
fast/workers/worker-cloneport.html
fast/workers/worker-close-more.html
fast/workers/worker-close.html
fast/workers/worker-context-multi-port.html
fast/workers/worker-gc2.html
fast/workers/worker-lifecycle.html
fast/workers/worker-messageport-gc.html
fast/workers/worker-multi-port.html
fast/workers/worker-terminate.html
fast/workers/storage/change-version-sync.html
fast/workers/storage/interrupt-database.html
fast/xmlhttprequest/null-document-xmlhttprequest-open.html
fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html

[David Levin]

Comment on attachment 77034 [details]
Attempt to fix crashes #2

Based on comments in the bug, this patch still causes problems.

[Csaba Osztrogonác]

(In reply to comment #12)
> (In reply to comment #11)
> > Created an attachment (id=77034) [details] [details]
> > Attempt to fix crashes #2
> > 
> > Changes from the version that was committed and reverted:
> > 
> > FrameLoaderClientQt.cpp : Use m_frame->loader()->activeDocumentLoader() instead of m_frame->document()->loader().  I believe that this will be safer if we catch the loader in the middle of a navigation.
> > 
> > FrameLoaderClientGtk.h/cpp : Use an m_hasRepresentation boolean in the same way the qt and chromium FrameLoaderClients do.  This should keep us from blowing away our DocumentLoader in FrameLoader::init().
> > 
> > mrobinson and ossy, could I trouble you to verify this patch on your respective platforms at your leisure?
> 
> I tried this patch with QtWebKit. The previously crashed API test passes, 
> but unfortunately it caused a flakey layout test regression. Sometimes 
> (every second or third execution of run-webkit-tests) there are 24 failing
> tests with this error message "FAIL: Timed out waiting for notifyDone to be
> called"
> 
> list of failing tests:
> fast/tokenizer/flush-characters-in-document-write-evil.html
> fast/tokenizer/flush-characters-in-document-write.html
> fast/viewport/viewport-128.html
> fast/workers/dedicated-worker-lifecycle.html
> fast/workers/shared-worker-exception.html
> fast/workers/shared-worker-lifecycle.html
> fast/workers/shared-worker-name.html
> fast/workers/shared-worker-script-error.html
> fast/workers/stress-js-execution.html
> fast/workers/termination-early.html
> fast/workers/termination-with-port-messages.html
> fast/workers/worker-cloneport.html
> fast/workers/worker-close-more.html
> fast/workers/worker-close.html
> fast/workers/worker-context-multi-port.html
> fast/workers/worker-gc2.html
> fast/workers/worker-lifecycle.html
> fast/workers/worker-messageport-gc.html
> fast/workers/worker-multi-port.html
> fast/workers/worker-terminate.html
> fast/workers/storage/change-version-sync.html
> fast/workers/storage/interrupt-database.html
> fast/xmlhttprequest/null-document-xmlhttprequest-open.html
> fast/xmlhttprequest/xmlhttprequest-nonexistent-file.html

Sorry, it was false positive alarm. :(( It caused by a Qt-DRT bug: https://bugs.webkit.org/show_bug.cgi?id=42578#c34

I retested your patch again (Attempt to fix crashes #2) and it works on Qt now. (I hade to resolve conflicts in 2 files to make the patch appliable)

[Nate Chapin]

Created attachment 78852 [details]
Merge previous patch to trunk

Thanks for the update on the state of the qt piece of this!

It sounds like that means there are no more known concerns with this patch.  I've uploaded a new version which is identical to the old one, but merged to tip of tree.

[Adam Barth]

Comment on attachment 78852 [details]
Merge previous patch to trunk

Ok.  This code will probably need some more merging, but no need to upload for review again.

[Nate Chapin]

http://trac.webkit.org/changeset/78342