Bug 30879 - Loading HTML with a JS alert() when the DocumentLoader has been set to not defer data load results in ASSERT
Summary: Loading HTML with a JS alert() when the DocumentLoader has been set to not de...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P3 Normal
Assignee: Tor Arne Vestbø
URL:
Keywords:
Depends on:
Blocks: 29381
  Show dependency treegraph
 
Reported: 2009-10-28 14:17 PDT by Tor Arne Vestbø
Modified: 2010-08-03 08:46 PDT (History)
9 users (show)

See Also:


Attachments
Clear the initial request when loading synchronously to prevent duplicate loads (1.92 KB, patch)
2009-10-30 06:14 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Patch with test (39.88 KB, patch)
2010-07-07 09:22 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Patch (38.00 KB, patch)
2010-07-08 03:42 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Patch (38.01 KB, patch)
2010-07-12 06:36 PDT, Tor Arne Vestbø
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 2009-10-28 14:17:33 PDT
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 :)
Comment 1 Tor Arne Vestbø 2009-10-29 07:37:35 PDT
Related to bug 29381
Comment 2 Darin Adler 2009-10-29 12:10:11 PDT
(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.
Comment 3 Tor Arne Vestbø 2009-10-30 06:14:54 PDT
Created attachment 42207 [details]
Clear the initial request when loading synchronously to prevent duplicate loads
Comment 4 Darin Adler 2009-10-30 10:36:05 PDT
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.
Comment 5 Tor Arne Vestbø 2009-10-30 10:55:11 PDT
(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?
Comment 6 Adam Barth 2009-11-16 15:04:54 PST
Oops.  Wrong bug.  Sorry.
Comment 7 Adam Barth 2009-11-30 12:21:52 PST
Attachment 42207 [details] passed the style-queue
Comment 8 Eric Seidel (no email) 2009-12-14 13:31:40 PST
This sounds vaguely related to bug 30989, but I doubt that it's actually related.
Comment 9 Maciej Stachowiak 2009-12-28 18:11:20 PST
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.
Comment 10 Maciej Stachowiak 2009-12-28 18:15:30 PST
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.
Comment 11 Tor Arne Vestbø 2010-01-05 06:21:24 PST
(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 :)
Comment 12 Darin Adler 2010-01-06 10:06:55 PST
(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.
Comment 13 Tor Arne Vestbø 2010-07-07 09:22:12 PDT
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.
Comment 14 Tor Arne Vestbø 2010-07-07 09:52:20 PDT
Spelling mistake in the test, ill fix that before landing
Comment 15 Adam Roben (:aroben) 2010-07-07 09:52:26 PDT
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.
Comment 16 WebKit Review Bot 2010-07-07 17:00:27 PDT
Attachment 60741 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3378430
Comment 17 Tor Arne Vestbø 2010-07-08 03:42:41 PDT
Created attachment 60863 [details]
Patch
Comment 18 Tor Arne Vestbø 2010-07-08 03:46:12 PDT
(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 :/
Comment 19 WebKit Review Bot 2010-07-08 08:49:51 PDT
Attachment 60863 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/3439039
Comment 20 Tor Arne Vestbø 2010-07-12 06:36:20 PDT
Created attachment 61221 [details]
Patch
Comment 21 Simon Hausmann 2010-07-12 13:38:54 PDT
Adam, can you help review this patch?

Thanks :)
Comment 22 WebKit Review Bot 2010-07-13 04:05:45 PDT
http://trac.webkit.org/changeset/63191 might have broken Qt Linux Release
Comment 23 Tor Arne Vestbø 2010-07-13 04:23:05 PDT
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
Comment 24 Simon Hausmann 2010-07-26 01:31:26 PDT
Committed r63191: <http://trac.webkit.org/changeset/63191>
Comment 25 Simon Hausmann 2010-07-26 06:57:19 PDT
Revision r63191 cherry-picked into qtwebkit-2.0 with commit 9923d5f69ae0dc413fe9c2b3e702b47887c19df5
Comment 26 Simon Hausmann 2010-08-03 08:46:09 PDT
Revision r63191 cherry-picked into qtwebkit-2.1 with commit 502eff516377d2f6ac6c56bcb055ee16b56e76bd