Bug 29690 - ApplicationCache Events Should Delay Firing Until After Document Load
Summary: ApplicationCache Events Should Delay Firing Until After Document Load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Nordman
URL: http://bogojoker.com/webkit/bug_onche...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-23 13:08 PDT by Joseph Pecoraro
Modified: 2010-01-28 22:41 PST (History)
6 users (show)

See Also:


Attachments
defer events until onload (9.02 KB, patch)
2010-01-25 15:58 PST, Michael Nordman
no flags Details | Formatted Diff | Diff
style fix (9.02 KB, patch)
2010-01-25 16:10 PST, Michael Nordman
no flags Details | Formatted Diff | Diff
more style fixes (9.04 KB, patch)
2010-01-25 16:19 PST, Michael Nordman
no flags Details | Formatted Diff | Diff
adds a test for a null documentLoader (9.07 KB, patch)
2010-01-26 11:10 PST, Michael Nordman
no flags Details | Formatted Diff | Diff
handles deletion while raising deferred events (12.75 KB, patch)
2010-01-26 18:46 PST, Michael Nordman
no flags Details | Formatted Diff | Diff
removed 6 space characters (12.74 KB, patch)
2010-01-26 20:00 PST, Michael Nordman
no flags Details | Formatted Diff | Diff
cleaner patch (11.35 KB, patch)
2010-01-27 15:11 PST, Michael Nordman
ap: review-
Details | Formatted Diff | Diff
rename a datamember and switch to Vector (11.32 KB, patch)
2010-01-28 17:10 PST, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2009-09-23 13:08:23 PDT
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.
Comment 1 Joseph Pecoraro 2009-09-23 13:17:36 PDT
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.
Comment 2 Alexey Proskuryakov 2009-09-25 09:36:03 PDT
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.
Comment 3 Joseph Pecoraro 2009-09-25 09:50:56 PDT
> 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!
Comment 4 Joseph Pecoraro 2010-01-12 18:16:52 PST
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.
Comment 5 Michael Nordman 2010-01-25 15:58:31 PST
Created attachment 47373 [details]
defer events until onload
Comment 6 WebKit Review Bot 2010-01-25 16:02:42 PST
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.
Comment 7 Michael Nordman 2010-01-25 16:10:36 PST
Created attachment 47374 [details]
style fix
Comment 8 Michael Nordman 2010-01-25 16:19:47 PST
Created attachment 47375 [details]
more style fixes
Comment 9 Alexey Proskuryakov 2010-01-25 17:36:05 PST
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.
Comment 10 Michael Nordman 2010-01-25 18:06:49 PST
>  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.
Comment 11 Michael Nordman 2010-01-26 11:08:57 PST
> > 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.
Comment 12 Michael Nordman 2010-01-26 11:10:20 PST
Created attachment 47421 [details]
adds a test for a null documentLoader
Comment 13 Michael Nordman 2010-01-26 12:52:46 PST
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?
Comment 14 Michael Nordman 2010-01-26 13:16:53 PST
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()".
Comment 15 Michael Nordman 2010-01-26 15:42:46 PST
> 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.
Comment 16 Michael Nordman 2010-01-26 16:35:20 PST
> 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.
Comment 17 Michael Nordman 2010-01-26 18:46:08 PST
Created attachment 47484 [details]
handles deletion while raising deferred events
Comment 18 WebKit Review Bot 2010-01-26 18:50:23 PST
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.
Comment 19 Michael Nordman 2010-01-26 20:00:52 PST
Created attachment 47485 [details]
removed 6 space characters
Comment 20 Michael Nordman 2010-01-27 13:59:37 PST
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.
Comment 21 Michael Nordman 2010-01-27 15:11:29 PST
Created attachment 47566 [details]
cleaner patch
Comment 22 Alexey Proskuryakov 2010-01-28 15:34:31 PST
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.
Comment 23 Michael Nordman 2010-01-28 17:10:01 PST
Created attachment 47658 [details]
rename a datamember and switch to Vector
Comment 24 Alexey Proskuryakov 2010-01-28 17:27:40 PST
Comment on attachment 47658 [details]
rename a datamember and switch to Vector

r=me
Comment 25 WebKit Commit Bot 2010-01-28 22:41:46 PST
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>
Comment 26 WebKit Commit Bot 2010-01-28 22:41:54 PST
All reviewed patches have been landed.  Closing bug.