Bug 32047

Summary:

Make appcache layouttests top-frame-*.html more robust for async checking event

Product:

WebKit

Reporter:

Jenn Braithwaite <jennb>

Component:

Tools / Tests

Assignee:

Michael Nordman <michaeln>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, dimich, jennb, levin, michaeln

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

OS X 10.5

Attachments:

Description	Flags
Proposed fix	ap: review-
Patch.	ap: review+, dimich: commit-queue-

Jenn Braithwaite

Reported 2009-12-01 18:25:26 PST

top-frame-2.html fails in chromium's appcache implementation because the checking event is asynchronous and may arrive after the iframe has posted the message back to its parent. The test ends as soon as it gets the message from the iframe, causing the checking event to be missed. Change the test to check for both a checking event and the receipt of the iframe's message before ending to ensure that both events are processed before calling the test a success.

Attachments
Proposed fix (2.32 KB, patch) 2009-12-02 16:31 PST, Jenn Braithwaite	ap: review-	Details Formatted Diff Diff
Patch. (6.56 KB, patch) 2010-03-11 14:36 PST, Dmitry Titov	ap: review+ dimich: commit-queue-	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2009-12-01 23:57:16 PST

The checking event is always asynchronous, not just in the Chromium implementation. Are you saying that the order in which the checking event and the message event arrive is undefined in Chromium? Would it be just a bug?

Jenn Braithwaite

Comment 2 2009-12-02 15:53:20 PST

In chromium, the appcache update process runs in the IO thread. Sequence of events for this test is as follows (on windows): UI thread adds iframe (subframe-2.html), triggering appcache update on IO thread. Update process on IO thread posts checking event to UI thread. Meanwhile, on UI thread, subframe-2.html finishes loading and executes the script. The XMLHttpRequest synchronously fails and calls parent.postMessage. postMessage uses startTimer(0) to post the message. The timer fires before the UI thread has a chance to process any messages in the thread queue. Thus, the message event listener in the parent(top-frame-2.html) invokes notifyDone() before the UI thread has a chance to process the checking event. Dimich tells me that depending on the platform, the firing of a timer event may or may not have priority over queued messages for a thread. So, the test should not depend on a particular ordering of the checking event over the message event.

Alexey Proskuryakov

Comment 3 2009-12-02 16:04:22 PST

The fact that postMessage() uses a timer is an implementation detail. How events are delivered is actually specified in HTML5 - we'll need to check if the test is correct per the spec to determine if it's the test or Chromium that needs to be fixed. It is also possible that HTML5 will need to be changed due to Chromium implementation experience.

Jenn Braithwaite

Comment 4 2009-12-02 16:31:57 PST

Created attachment 44193 [details] Proposed fix The spec says to "queue a task" to fire the checking event. The definition of "queue a task" in the spec does not impose an ordering on script events vs appcache update events. Attached patch makes the test robust against ordering of these events.

Alexey Proskuryakov

Comment 5 2009-12-02 16:39:45 PST

Comment on attachment 44193 [details] Proposed fix (In reply to comment #4) > The spec says to "queue a task" to fire the checking event. The definition of > "queue a task" in the spec does not impose an ordering on script events vs > appcache update events. Application cache update events are fired in exactly the same way ("queue a task to fire a simple event named checking"), which means that they go into the same FIFO queue, and must be delivered sequentially.

Michael Nordman

Comment 6 2009-12-02 17:50:37 PST

(In reply to comment #5) > (From update of attachment 44193 [details]) > (In reply to comment #4) > > The spec says to "queue a task" to fire the checking event. The definition of > > "queue a task" in the spec does not impose an ordering on script events vs > > appcache update events. > > Application cache update events are fired in exactly the same way ("queue a > task to fire a simple event named checking"), which means that they go into the > same FIFO queue, and must be delivered sequentially. If I'm understanding this properly, the difference is that in Chrome the CHECKING event is queued and eventually raised some time after the frame load has made progress, whereas in Safari the CHECKING event is queued prior to the frame load getting past the <html> tag parsing and is guaranteed to be raised prior to any events queued after that point in time. And the layout test depends on that synchronous queueing guarantee. Here's the relavant part of the HTML5 spec... <snip> The application cache download process steps are as follows: 1) Optionally, wait until the permission to start the application cache download process has been obtained from the user and until the user agent is confident that the network is available. This could include doing nothing until the user explicitly opts-in to caching the site, or could involve prompting the user for permission. The algorithm might never get past this point. (This step is particularly intended to be used by user agents running on severely space-constrained devices or in highly privacy-sensitive environments). 2) Atomically, so as to avoid race conditions, perform the following substeps blah, blah, queue a CHECKING event, blah </snip> Step 1 implies that the entire UpdateAlgorithm (including the raising of the CHECKING event) can be deferred for an arbitrarily long period of time, presumably without preventing a frame load from continuing. Chrome respects the "Atomically, so as to avoid race conditions" part of this steps to be sure. But my reading of this is that "Atomically" should not be interpreted as "Synchronously". So, I think Chrome's more async behavior in this area is valid per the spec today. Also, if the spec did not allow some async'ness here... i'd be looking to change that in the spec. Sure, we *could* force things to be synchronous, but that would only slow page loading (which we generally have an aversion to).

Alexey Proskuryakov

Comment 7 2009-12-02 23:49:53 PST

Interesting! This step 1 didn't exist when I was making this test. I'll need to think about consequences some more - as you noticed, it's quite close to being in contradiction with atomicity from step 2.

Michael Nordman

Comment 8 2009-12-03 11:08:23 PST

(In reply to comment #7) > Interesting! This step 1 didn't exist when I was making this test. I'll need to > think about consequences some more - as you noticed, it's quite close to being > in contradiction with atomicity from step 2. Step 1 allows for an async user prompt while the page continues to load. I think that's probably a good things since it gives user agents some wiggle room on how to present this form of persistence to the user. Chrome doesn't have any UI yet, but you can imagine an "info bar" that appears when a particular manifest file is first referred to and the update algorithm waits until the DOIT button is pressed (maybe). UI deliberations aside, accommodating async'ness here is good for other reasons as well. These steps are being invoked in the act of <html> tag parsing (in the act of cache-selection). The cache selection part of may involves disk IO. We all generally prefer to decouple the UI thread from disk IO. As mentioned before, my reading of the spec is that async'ness here is permitted. I'm not at all keen on making our impl sync here for the reasons mentioned above (and other performance related reasons not mentioned above).

Alexey Proskuryakov

Comment 9 2009-12-04 14:05:54 PST

I agree for the most part. The spec doesn't look fully updated for this change - right after step 2, there's "The remainder of the steps run asynchronously." I was worried about behavioral changes caused by this that can affect existing content, but I can't find any. I sent e-mail to WhatWG, asking to clarify whether the text after step 2 is a mistake. It this really the only test affected by this spec change? I'd like to avoid having to fix them one by one. Could you perhaps add a sleep for one second to the beginning of cache update algorithm in Chromium implementation to see if that breaks more tests?

Jenn Braithwaite

Comment 10 2009-12-04 14:12:56 PST

top-frame-3.html and top-frame-4.html are both affected as well, but will usually pass as they run longer and the checking events usually arrive before the test ends.

Dmitry Titov

Comment 11 2010-03-10 17:01:36 PST

The top-frame2.html was already modified as proposed here in the patch for bug 35943. So the remaining work is to verify/update top-frame3 and top-frame4

Dmitry Titov

Comment 12 2010-03-11 14:29:23 PST

In fact, more tests in http/tests/appcache folder are using a combination of appcache events and postMessage to determine the termination time and success of the test. Take fro example the remove-cache.html. It actually registers appcache event handlers that *should not* fire and considers it a failure if they fire. However, it uses postMessage from included iframe as a termination signal. If the appcache actually fires those 'wrong' events but onmessage terminates the test right before this, we might miss a bug. Apparently the spec, as indicated in the discussion in bug 35943, assumes no ordering of events between different event sources. That could make tests dependin gon the particular ordering always pass on one implementation (or port) and fail on another, or, worse, be flaky. I'd like to try to make top-frame-*.html tests at least be consistent with each other and test for all conditions before termination. The patch for that is coming. Also, I think we need a separate bug for the appcache folder, to review and perhaps update all the affected tests. Apparently, some of them are disabled for Chromium port specifically because of the event ordering mismatch.

Dmitry Titov

Comment 13 2010-03-11 14:36:11 PST

Created attachment 50542 [details] Patch.

Alexey Proskuryakov

Comment 14 2010-03-11 14:49:49 PST

Comment on attachment 50542 [details] Patch. > Also added check for "noupdate" event. I'm not sure if I understand this change. What you are doing here is replacing a handler that calls test() with one that logs and checks whether the test is done. The counter doesn't match the number of events received, because the original handler doesn't increment it. applicationCache.onnoupdate = function() { test() } applicationCache.oncached = function() { test() } Do the tests still pass in browser both on first try (with empty appcache), and on reload? r=me though.

Dmitry Titov

Comment 15 2010-03-11 15:11:23 PST

(In reply to comment #14) > (From update of attachment 50542 [details]) > > Also added check for "noupdate" event. > > I'm not sure if I understand this change. What you are doing here is replacing > a handler that calls test() with one that logs and checks whether the test is > done. The counter doesn't match the number of events received, because the > original handler doesn't increment it. > > applicationCache.onnoupdate = function() { test() } > applicationCache.oncached = function() { test() } > > Do the tests still pass in browser both on first try (with empty appcache), and > on reload? Yes, they pass in both conditions. There are 3 opportunities for those events to fire - first time, when the page itself loads, then both of those tests attach 2 iframes. So the first time onnoupdate fires as a result of the page and calls test(), after that it fires twice for each iframe. The test counts up to 2.

Dmitry Titov

Comment 16 2010-03-11 15:14:59 PST

Landed: http://trac.webkit.org/changeset/55858

Note You need to log in before you can comment on or make changes to this bug.