Bug 29690 – ApplicationCache Events Should Delay Firing Until After Document Load

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Joseph Pecoraro]

When a page has a cache manifest and includes an external script (<script src="...">) regardless of wether or not that external script loads, any "checking" event listeners on the Application Cache do not fire.  This differs from Firefox, which (as I would have expected) throws the checking event listeners.

Test Cases: 
Error Page: http://bogojoker.com/webkit/bug_onchecking/bad.html
Success Page: http://bogojoker.com/webkit/bug_onchecking/good.html

Steps to Reproduce:
1. Navigate to the error test page.
2. Refresh the page (see below for the reason).

Actual Results:
No alert dialog, the listener was not run.

Expected Results:
The onchecking listener should fire and an alert dialog saying "checking" should have appeared.

Why You Had to Refresh:
This is another difference between Firefox and WebKit. Due to my testing when initially loading a page and on subsequent loads of a page with a manifest Firefox will fire the onchecking event. WebKit only fires the onchecking event on subsequent loads.

[Joseph Pecoraro]

Ahh, you can ignore the "Why You Had to Refresh" section. I did a complete test from scratch and WebKit does correctly fire onchecking on the first load (as per the spec):
http://www.whatwg.org/specs/web-apps/current-work/#event-appcache-checking

But the bug that I talk about here still exists.

[Alexey Proskuryakov]

Here is what's going on:
1) As soon as body manifest attribute is processed, application cache update begins, and a zero-timer task to dispatch a checking event is posted for later execution.
2) <script src="THIS_FILE_DOES_NOT_EXIST"> begins to load, blocking the inline script below that sets applicationCache.onchecking.
3) While THIS_FILE_DOES_NOT_EXIST is being requested, the task from step 1 fires, and the checking event gets dispatched, even though there is no listener set for it yet.
4) Loading finally finishes with a failure, so main document parsing resumes. Onchecking listener gets set, but it's too late now.

This might actually be correct behavior, I'm not sure yet. And this is not limited to appcache - all the new events that are posted asynchronously can get dispatched even when parsing is paused for an external script.

[Joseph Pecoraro]

> This might actually be correct behavior, I'm not sure yet. And this is not
> limited to appcache - all the new events that are posted asynchronously can get
> dispatched even when parsing is paused for an external script.

I'd be worried if this is the correct behavior.   If that is what the spec says then maybe it should be fixed / clarified. A more realistic scenario then the test case I provided is if there is applicationCache logic in the external javascript file. For instance a simple library that interacts with the applicationCache in order to provide users with a custom user interface (as 6.9.4.4 mentions a default User Agent Interface) indicating what is happening.

In this case I wouldn't want the applicationCache's checking event to fire before that external script, which would contain handlers for these types of events, is downloaded!

[Joseph Pecoraro]

Following up some recent discussion.

Some discussion was made on the Mailing List:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2010-January/024741.html

Discussion produces the following change:
http://html5.org/tools/web-apps-tracker?from=4581&to=4582

Summary:

> Done. All the events fired during the update process are delayed if the 
> Document whose ApplicationCache they are targetting hasn't loaded yet.

[Michael Nordman]

Created attachment 47373 [details]
defer events until onload

[WebKit Review Bot]

Attachment 47373 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/loader/appcache/ApplicationCacheHost.cpp:253:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Created attachment 47374 [details]
style fix

[Michael Nordman]

Created attachment 47375 [details]
more style fixes

[Alexey Proskuryakov]

Comment on attachment 47375 [details]
more style fixes

I don't have the time to deeply look at this right away, but something I noticed while looking through the patch quickly:

 void FrameLoader::handledOnloadEvents()
 {
     m_client->dispatchDidHandleOnloadEvents();
+#if ENABLE(OFFLINE_WEB_APPLICATIONS)
+    documentLoader()->applicationCacheHost()->didHandleDocumentOnloadEvents();

I don't think didHandleDocumentOnloadEvents() is a good name - it doesn't tell anything at all at call site, and only tells a little at implementation. The method name should say what it does.

Did you run existing tests? I'm mildly surprised that none of them broke.

[Michael Nordman]

>  void FrameLoader::handledOnloadEvents()
>  {
>      m_client->dispatchDidHandleOnloadEvents();
> +#if ENABLE(OFFLINE_WEB_APPLICATIONS)
> +    documentLoader()->applicationCacheHost()->didHandleDocumentOnloadEvents();
> 
> I don't think didHandleDocumentOnloadEvents() is a good name - it doesn't tell
> anything at all at call site, and only tells a little at implementation. The
> method name should say what it does.

I thought about naming from the point of view of the appcachehost (something along the lines of 'enableEventDispatching()') but ended up sticking to the naming scheme that indicates 'did handle onload event' for two reasons...

1) That scheme is used throughout the other classes that get poked after this has happened, ala...
  FrameLoader::handledOnloadEvents()
  FrameLoaderClient::dispatchDidHandleOnloadEvents()

2) That is the specified thing of interest that has to happen prior to deferred events being surfaced into the page. Having the method named in those terms on the ApplicationCacheHost class makes that more clear.

I'm open to suggestions.

> Did you run existing tests? I'm mildly surprised that none of them broke.

Yes, I was pleasantly surprised to see them all pass, but sure enough, they did.

[Michael Nordman]

> > Did you run existing tests? I'm mildly surprised that none of them broke.
> 
> Yes, I was pleasantly surprised to see them all pass, but sure enough, they
> did.

Wait, I had just run the appcache tests which happily passed. But there are some crashers in the full set of tests in cases where the frameLoader has no documentLoader. New patch coming soon with a test for null which passes the full set of tests.

[Michael Nordman]

Created attachment 47421 [details]
adds a test for a null documentLoader

[Michael Nordman]

Some thoughts about renaming the new methods/members of ApplicationCacheHost...

void stopDeferringEvents();  // also raises the events that have been queued up

bool m_isDeferringEvents;  // initialized to true
Deque<EventID> m_deferredEvents;

wdyt?

[Michael Nordman]

I think what's there works for webcore/loader/appcache and chrome/appcache alike. I didn't see a better hook for 'onload has happened' than the one i hooked into.

One concern i have, is it possible that a handler will result in DocumentLoader and its ApplicationCacheHost object being deleted while we're looping over the queue events and raising them?

Fyi, as coded, this is happening while the document is still "processingLoadEvent()".

[Michael Nordman]

> One concern i have, is it possible that a handler will result in DocumentLoader
> and its ApplicationCacheHost object being deleted while we're looping over the
> queue events and raising them?

Well... yes it is possible for that to happen... the following gives the current patch trouble...

<html manifest="THIS_FILE_DOES_NOT_EXIST.manifest">
<head>
  <script src="THIS_FILE_DOES_NOT_EXIST.js" type="text/javascript"></script>
  <script type="text/javascript">
    window.applicationCache.onchecking = function() {
        if (window != window.top)
            window.top.killFrame();
    }

    window.applicationCache.onerror = function() {
        throw "crash damnit!";
    }

    function killFrame() {
        document.body.removeChild(document.getElementsByTagName("iframe")[0]);
    }
  </script>
</head>
<body onload="onloadHasBeenCalled = true">
Do whacky things and try to crash the system.
<iframe src="deferred-events-try-to-crash.html"></iframe>
</body>
</html>


Some options for how to handle this sort of case.

1) Very directly inform the loop doing the event raising that the instance has been deleted and have it stop looping.
   - put a local bool on the stack, set to true (shouldContinueRaisingDeferredEvents)
   - put a ptr to that local in a data member (generally this ptr value is null,
     only non-null while looping)
   - dtor checks ptr value and if not null sets the value on the stack to 'false'
   - loop tests the value on the stack prior to continuing loop

2) AddRef DocumentLoader while running the loop, so we don't get deleted in the act.

3) Pick off one deferred event at a time and raise it. Go thru the message loop for each one. If deleted, stop.

The first is the most focused way to resolve the problem i'm seeing. The other two change life cycle of instances and/or make event delivery less deterministic.

[Michael Nordman]

> The first is the most focused way to resolve the problem i'm seeing.

I've implemented that first option to deal with the delete while looping, that fix works all right.

I'll prepare a new patch with that fix, and renaming as outlined earlier, and a new test that tickles the crash i was seeing.

[Michael Nordman]

Created attachment 47484 [details]
handles deletion while raising deferred events

[WebKit Review Bot]

Attachment 47484 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/loader/appcache/ApplicationCacheHost.cpp:253:  One space before end of line comments  [whitespace/comments] [5]
WebCore/loader/appcache/ApplicationCacheHost.cpp:264:  One space before end of line comments  [whitespace/comments] [5]
WebCore/loader/appcache/ApplicationCacheHost.h:114:  One space before end of line comments  [whitespace/comments] [5]
WebCore/loader/appcache/ApplicationCacheHost.h:122:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/ApplicationCacheHost.cpp:218:  One space before end of line comments  [whitespace/comments] [5]
WebKit/chromium/src/ApplicationCacheHost.cpp:229:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Created attachment 47485 [details]
removed 6 space characters

[Michael Nordman]

I've also tested a different solution to the the problem with the frame being removed while raising the deferred events. Simply addref the DocumentLoader while running the loop so the DocumentLoader and its ApplicationCacheHost don't get finally released in that loop.

This passes the layout tests as well and i think is cleaner code. When the frame is removed from its container while iterating over the events, things get detached nicely w/o finally deleting the objects until the loop exits.

I'll upload a patch with that solution soon.

[Michael Nordman]

Created attachment 47566 [details]
cleaner patch

[Alexey Proskuryakov]

Comment on attachment 47566 [details]
cleaner patch

This looks very good. Two minor nitpicks Michael agreed to address:

I'd have used a Vector, not a Deque - generally, the former is the best solution for most data structures, as well as there aren't too many items expected. It has simpler housekeeping, avoids executable code bloat, and asymptotic performance characteristics don't matter yet. In WebKit, Deque is implemented with a Vector anyway.

Also, m_isDeferringEvents should be m_defersEvents for consistency with m_defersCallbacks elsewhere in the code base.

[Michael Nordman]

Created attachment 47658 [details]
rename a datamember and switch to Vector

[Alexey Proskuryakov]

Comment on attachment 47658 [details]
rename a datamember and switch to Vector

r=me

[WebKit Commit Bot]

Comment on attachment 47658 [details]
rename a datamember and switch to Vector

Clearing flags on attachment: 47658

Committed r54044: <http://trac.webkit.org/changeset/54044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.