(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.

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().

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?

(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

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.