Bug 30879 – Loading HTML with a JS alert() when the DocumentLoader has been set to not defer data load results in ASSERT

• Component: Page Loading
• Status: RESOLVED FIXED

[Tor Arne Vestbø]

Qt's QWebFrame::setHtml() uses DocumentLoader::setDeferMainResourceDataLoad(false) to force synchronuous loading:

WTF::PassRefPtr<WebCore::DocumentLoader> FrameLoaderClientQt::createDocumentLoader(const WebCore::ResourceRequest& request, const SubstituteData& substituteData)
{
    RefPtr<DocumentLoader> loader = DocumentLoader::create(request, substituteData);
    if (substituteData.isValid())
        loader->setDeferMainResourceDataLoad(false);
    return loader.release();
}

When the subsituteData that's being loaded has a piece of JS that does an alert("foo"), we end up in Chrome::runJavaScriptAlert(), which then uses a PageGroupLoadDeferrer to protect against paralell event loops:

// Defer loads in case the client method runs a new event loop that would
// otherwise cause the load to continue while we're in the middle of executing JavaScript.
PageGroupLoadDeferrer deferrer(m_page, true);

After the alert() dialog has been closed by the user, the PageGroupLoadDeferrer goes out of scope, resulting in a call to MainResourceLoader::setDefersLoading(false).

The problem as I understand it is that MainResourceLoader::setDefersLoading() does not take into account the case where the resources has already been loaded, so it starts a new load, either though loadNow() or through the timer.

My fix so far is to use m_response as a sign that the request has already been loaded:

http://gist.github.com/220820

My questions are:

   1. Is my assumption about the bug correct?
   2. If so, is that the right sign to use?
   3. Is it in the right place? I.e should it be moved to loadNow() et al?
   
I see ResourceLoader has a check for  if (!defers && !m_deferredRequest.isNull()), perhaps that's what's preventing this bug from appearing there?
   
Thanks for any input :)

[Tor Arne Vestbø]

Related to bug 29381

[Darin Adler]

(In reply to comment #0)
> The problem as I understand it is that MainResourceLoader::setDefersLoading()
> does not take into account the case where the resources has already been
> loaded, so it starts a new load, either though loadNow() or through the timer.

The logic in MainResourceLoader seems a bit twisted and has changed quite a bit since I last looked at it.

The reason m_initialRequest exists is to record that there's deferred loading that needs to be done. It should be cleared when the resource has already been loaded. I don't think adding a check for m_response is the right thing to do.

Adding code to clear m_initialRequest in handleDataLoadNow is probably the right fix.

[Tor Arne Vestbø]

Created attachment 42207 [details]
Clear the initial request when loading synchronously to prevent duplicate loads

[Darin Adler]

Comment on attachment 42207 [details]
Clear the initial request when loading synchronously to prevent duplicate loads

Is there any way to make a regression test for this? It mist affect cases other than just a particular way of calling WebKit in Qt.

If not, why not?

Adam Barth would probably be made at me if I said review+ on this without first getting an answer to that.

By the way, not about this patch, just about the existing code: I think it's really confusing that ResourceLoader has a member called m_deferredRequest and this class has a member called m_initialRequest and they have almost the same purpose.

[Tor Arne Vestbø]

(In reply to comment #4)
> (From update of attachment 42207 [details])
> Is there any way to make a regression test for this? It must affect cases other than just a particular way of calling WebKit in Qt.

As far as I can tell we're the only ones calling setDeferMainResourceDataLoad(false), apart from the original code that introduced it in r23627, where it seems to set m_deferMainResourceDataLoad = false in WebDocumentLoaderMac when detecting com.apple.AppKit or an Adobe Installer.

Would it be possible to add a DRT hook for this for layout tests, where the test would first disable the deferred load and then trigger another load from the test?

> Adam Barth would probably be made at me if I said review+ on this without first
> getting an answer to that.

Would love to know if there's some way to test this :)

> By the way, not about this patch, just about the existing code: I think it's
> really confusing that ResourceLoader has a member called m_deferredRequest and
> this class has a member called m_initialRequest and they have almost the same
> purpose.

I agree, i noticed this too when you tipped med off of the real intent behind initialRequest. I could rename this in an upcoming patch if you want?

[Adam Barth]

Oops.  Wrong bug.  Sorry.

[Adam Barth]

Attachment 42207 [details] passed the style-queue

[Eric Seidel (no email)]

This sounds vaguely related to bug 30989, but I doubt that it's actually related.

[Maciej Stachowiak]

Comment on attachment 42207 [details]
Clear the initial request when loading synchronously to prevent duplicate loads

The code change looks good to me, but this needs a layout test.

[Maciej Stachowiak]

Comment on attachment 42207 [details]
Clear the initial request when loading synchronously to prevent duplicate loads

I see that the test case issue was already discussed in comments. I see thrre reasonable options:

1) I believe a test case can be made by using an alert() during initial load, as described in the ChangeLog.
2) If the first approach doesn't work, adding a hook to DumpRenderTree is reasonable.
2) If it is truly impossible to test this, then the ChangeLog should explain why it's currently not possible to make a test case.

[Tor Arne Vestbø]

(In reply to comment #10)
> 1) I believe a test case can be made by using an alert() during initial load, as
> described in the ChangeLog.

My bad, the ChangeLog was not describing full picture. The alert() will only trigger the assert if the DocumentLoader has setDeferMainResourceDataLoad() set to false, which then results in a erroneous call to loadNow() in MainResourceLoader::setDefersLoading().

As far as I can tell deferred main resource loads are only used by Qt's QWebFrame::setHtml() API to do synchronous loads, as well as Chromium's WebWorker implementation.

> 2) If the first approach doesn't work, adding a hook to DumpRenderTree is
> reasonable.

I tried poking around to see if I could add a DRT hook for this. The setter/getter in LTC is of course easy, but I'm wondering where would be a good place to call DocumentLoader::setDeferMainResourceDataLoad(false), as the DRT does not seem to control the loader directly but uses the FrameLoadDelegate mechanism for hooks.

I considered adding a hook there for FrameLoaderClient::createDocumentLoader(), which is the place we setDeferMainResourceDataLoad(false) in the Qt port, but as I understand it WebFrameLoadDelegate is public API for the Mac and Win ports?

I also tried looking at hooking into willSendRequest of the WebResourceLoadDelegate, which gets passed a a WebDataSource, but it seems only the Windows COM API has a way to access the document loader though the WebDataSource, the Obj-C bindings's WebDataSource is opaque and has no documentLoader() method.

I'm probably missing something, so any hints on where to make the DRT access the loader and set the main resource to be non-deferred would be appreciated :)

[Darin Adler]

(In reply to comment #11)
> I tried poking around to see if I could add a DRT hook for this. The
> setter/getter in LTC is of course easy, but I'm wondering where would be a good
> place to call DocumentLoader::setDeferMainResourceDataLoad(false), as the DRT
> does not seem to control the loader directly but uses the FrameLoadDelegate
> mechanism for hooks.

Yes, DumpRenderTree uses mostly public API. You'd need to add new "SPI" to make this work on Mac OS X at least.

[Tor Arne Vestbø]

Created attachment 60741 [details]
Patch with test

Patch now has test, using two new DRT functions. 

Hoping the win-ews will catch build errors for this one as I don't have a win-machine readily available right now.

I'll file bugs for Chromium and GTK for the skipped test before landing the patch once it's accepted.

[Tor Arne Vestbø]

Spelling mistake in the test, ill fix that before landing

[Adam Roben (:aroben)]

Comment on attachment 60741 [details]
Patch with test

> +        COMPtr<IWebDataSourcePrivate> dataSourcePrivate;
> +        if (FAILED(dataSource->QueryInterface(&dataSourcePrivate)))
> +            return S_FAILED;
> +        dataSourcePrivate->setDeferMainResourceDataLoad(false);

This should be:

COMPtr<IWebDataSourcePrivate> dataSourcePrivate(Query, dataSource);
if (!dataSourcePrivate)
    return E_FAIL;
dataSourcePrivate->setDeferMainResourceDataLoad(FALSE);

Other than that, the Windows parts look fine.

[WebKit Review Bot]

Attachment 60741 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3378430

[Tor Arne Vestbø]

Created attachment 60863 [details]
Patch

[Tor Arne Vestbø]

(In reply to comment #17)
> Created an attachment (id=60863) [details]
> Patch

Updated:

 - Fixed spelling mistake in test
 - Incorporated Adam's feedback
 - Removed changes to WebKit.order (rumor is it will be auto-generated for me)

Dunno what the build error was on the Win-EWS bot, the logs do not say anything :/

[WebKit Review Bot]

Attachment 60863 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3439039

[Tor Arne Vestbø]

Created attachment 61221 [details]
Patch

[Simon Hausmann]

Adam, can you help review this patch?

Thanks :)

[WebKit Review Bot]

http://trac.webkit.org/changeset/63191 might have broken Qt Linux Release

[Tor Arne Vestbø]

Hmm, the win build-bot needs to be kicked somehow so that include/WebKit/WebKit.h is regenerated so that the build picks up the new private API

[Simon Hausmann]

Committed r63191: <http://trac.webkit.org/changeset/63191>

[Simon Hausmann]

Revision r63191 cherry-picked into qtwebkit-2.0 with commit 9923d5f69ae0dc413fe9c2b3e702b47887c19df5

[Simon Hausmann]

Revision r63191 cherry-picked into qtwebkit-2.1 with commit 502eff516377d2f6ac6c56bcb055ee16b56e76bd