Bug 138506

Summary: Async XMLHttpRequest should get access to AppCache resources stored as flat files
Product: WebKit Reporter: Ashley Gullen <ashley>
Component: Web AudioAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Major CC: ap, beidson, buildbot, commit-queue, crogers, eric.carlson, japhet, jer.noble, rniwa, youennf
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://www.scirra.com/labs/bugs/iosajaxcache/
Attachments:
Description Flags
First stab
none
Archive of layout-test-results from ews102 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Fixing tests
none
Better integration
none
Trying to fix iOS build
none
WIP using platformStrategies
none
Patch
none
Async handling
none
Rebasing
none
Fixing code moved to ResourceLoader
none
Patch for landing none

Description Ashley Gullen 2014-11-07 09:47:07 PST
Steps to reproduce:

1. Visit http://www.scirra.com/labs/bugs/iosajaxcache/
2. Press 'Play sound'

If that works correctly, wait a few seconds, press Refresh, then press 'Play sound' again.

Expected result:

Sound should play every time 'Play sound' is pressed.

Observed result:

The AJAX request for explosion.m4a completes successfully, but returns an ArrayBuffer with a byteLength of 0. Audio decoding subsequently fails and the audio cannot play.

Compare to http://www.scirra.com/labs/bugs/iosajaxnocache/, which is identical except offline.appcache has been deleted and the html manifest attribute removed. Audio plays correctly every time.

Therefore an AJAX request of type "arraybuffer" to a file in the AppCache always returns an empty ArrayBuffer.

Reproduces with Safari on iOS 8.1, as well as Safari 8 on OS X 10.10 Yosemite. It works correctly in Chrome for Windows, Android and OS X, as well as Firefox for Windows. Note the test does not work on Firefox for OS X not because it is affected by this bug (it returns the correct byteLength), but simply because the test only uses an AAC audio file and Firefox does not support that format on all platforms.

Impact:

Any page using both AppCache and Web Audio will be broken, since the normal way of loading audio files to play with Web Audio does not work.

All content created by Construct 2 (www.scirra.com), a major HTML5 game engine, is affected by this issue. All the games it exports use AppCache for offline support and Web Audio for sound effect playback. There is already a large amount of Construct 2 content on the web affected by this, and the impact is that no sound plays in Safari on iOS or OS X.
Comment 1 Ashley Gullen 2015-03-10 03:42:55 PDT
Still reproduces on iOS 8.2!
Comment 2 Ashley Gullen 2015-04-09 09:49:40 PDT
Still reproduces on iOS 8.3. Please fix it! It stops anything using Web Audio from working offline!
Comment 3 youenn fablet 2015-04-10 03:51:49 PDT
(In reply to comment #2)
> Still reproduces on iOS 8.3. Please fix it! It stops anything using Web
> Audio from working offline!

Thanks for keeping pinging :)
I am not sure the issue comes from XMLHttpRequest, but more probably from AppCache storage implementation.

Looking at ApplicationCacheStorage::shouldStoreResourceAsFlatFile, I guess the same piece of code would work if serving the audio with an application/octet-stream mime type.
Comment 4 youenn fablet 2015-04-10 05:19:02 PDT
Created attachment 250509 [details]
First stab
Comment 5 Build Bot 2015-04-10 05:53:08 PDT
Comment on attachment 250509 [details]
First stab

Attachment 250509 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6042598003179520

New failing tests:
http/tests/appcache/simple.html
Comment 6 Build Bot 2015-04-10 05:53:12 PDT
Created attachment 250510 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-04-10 06:15:56 PDT
Comment on attachment 250509 [details]
First stab

Attachment 250509 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5035103902236672

New failing tests:
http/tests/appcache/simple.html
Comment 8 Build Bot 2015-04-10 06:15:59 PDT
Created attachment 250514 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 9 Eric Carlson 2015-04-10 08:25:10 PDT
Comment on attachment 250509 [details]
First stab

View in context: https://bugs.webkit.org/attachment.cgi?id=250509&action=review

> LayoutTests/http/tests/appcache/simple.html:59
> +    // Load a video resource that exists in the cache.
> +    try {
> +        var req = new XMLHttpRequest();
> +        req.open("GET", "resources/test.mp4", false);
> +        req.send();
> +    } catch (e) {
> +        errorTest("FAILURE: Could not load video data from cache");
> +        return;
> +    }
> +    if (req.response.length == 0) {
> +        errorTest("FAILURE: Did not get correct video data from cached resource");
> +        return;
> +    }

I think this should be split out into a new test.

> LayoutTests/http/tests/appcache/simple.html:78
> +    // Load asynchronously a video resource that exists in the cache.
> +    try {
> +        var req = new XMLHttpRequest();
> +        req.open("GET", "resources/test.mp4");
> +        req.send();
> +        req.onloadend = function() {
> +            if (req.response == 0) {
> +                errorTest("FAILURE: Did not get correct data from cached resource");
> +                return;
> +            }
> +            document.getElementById('result').innerHTML = "SUCCESS"
> +            if (window.testRunner)
> +                testRunner.notifyDone();
> +        }            
> +    } catch (e) {
> +        errorTest("FAILURE: Could not load video data from cache");
> +        return;
> +    }     

Ditto. It may be worth having separate tests for sync and async .
Comment 10 youenn fablet 2015-04-10 10:11:51 PDT
Created attachment 250520 [details]
Fixing tests
Comment 11 youenn fablet 2015-04-12 03:21:59 PDT
Created attachment 250597 [details]
Better integration
Comment 12 youenn fablet 2015-04-12 05:00:01 PDT
Created attachment 250600 [details]
Trying to fix iOS build
Comment 13 Darin Adler 2015-04-12 12:57:43 PDT
Comment on attachment 250600 [details]
Trying to fix iOS build

View in context: https://bugs.webkit.org/attachment.cgi?id=250600&action=review

It’s not right to read from the disk on the main thread. We need to do this asynchronously.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:203
> +static inline bool loadDataFromFile(const String& path, Vector<char>& data)

Can’t see a good reason to mark this inline. Doesn’t seem to need inlining for performance and it’s called in two places, so inlining will make code size bigger.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:207
> +    long long size = 0;

Don’t need to initialize this. The getFileSize function will set it if it succeeds.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:208
> +    if (!getFileSize(path, size)) {

Not sure why we are using FileStream for reading the file, but the global getFileSize for getting its size. We should consistently use the same class for both. Generally speaking I’m not sure we should be using the FileStream class for this at all. Currently it’s only used in BlobResourceHandle and I’m not sure we want to use it again here rather than using the lower level functions it is built on top of. FileStream has some strange stuff in it that we probably don’t want to use. Also, on most if not all out platforms, we can do mapFile instead of reading the file in.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:216
> +    if (stream.openForRead(path, 0, size)) {

If this returns false, why is the function returning true? I also think this would be easier to understand as an early return (like the one for size just above) rather than nesting the code to do the work inside this if.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:217
> +        data.resize(static_cast<size_t>(size));

This should be a call to the grow function rather than the resize function.

But also, this will just crash if the file is too big to load into memory. Is that the behavior we want?

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:220
> +            int read = stream.read(buffer, static_cast<int>(size));

The code here will do the wrong thing size is larger than the maximum int. In fact, if it’s 1 << 32 than I think we might even get an infinite loop. We should guard against that somehow.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:223
> +            size -= read;

This should go *after* the check for "read < 0".

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:225
> +                stream.close();

There’s no reason to explicitly close the stream, since destroying the stream takes care of that.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:231
> +        stream.close();

There’s no reason to explicitly close the stream, since destroying the stream takes care of that.

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:241
> +        if (!loadDataFromFile(resource->path(), data))

It’s not appropriate to do blocking I/O on the main thread for this.
Comment 14 youenn fablet 2015-04-13 10:58:38 PDT
(In reply to comment #13)
> Comment on attachment 250600 [details]
> Trying to fix iOS build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=250600&action=review
> 
> It’s not right to read from the disk on the main thread. We need to do this
> asynchronously.

OK.
Maybe ApplicationCacheHost should ask the NetworkProcess to load the corresponding file:// URL data then?

I will try to prepare a patch for it, probably splitting sync and async cases in separated patches.
Comment 15 Alexey Proskuryakov 2015-04-13 11:06:48 PDT
> Maybe ApplicationCacheHost should ask the NetworkProcess to load the corresponding file:// URL data then?

That would seem weird, given that nothing in AppCache depends on the NetworkProcess.
Comment 16 youenn fablet 2015-04-13 11:54:43 PDT
(In reply to comment #15)
> > Maybe ApplicationCacheHost should ask the NetworkProcess to load the corresponding file:// URL data then?
> 
> That would seem weird, given that nothing in AppCache depends on the
> NetworkProcess.

By NetworkProcess, I was meaning using platformStrategies to either load the file resource syncly or asyncly. In WK2, this would end up in the NetworkProcess.

Does that make more sense?
Comment 17 youenn fablet 2015-04-13 12:51:37 PDT
Created attachment 250668 [details]
WIP using platformStrategies
Comment 18 youenn fablet 2015-04-13 12:53:55 PDT
(In reply to comment #17)
> Created attachment 250668 [details]
> WIP using platformStrategies

Here is a first stab.
The resource loading could probably be shared at some point with ApplicationCacheGroup.
Comment 19 youenn fablet 2015-04-14 07:12:10 PDT
Created attachment 250703 [details]
Patch
Comment 20 youenn fablet 2015-04-29 02:43:18 PDT
Created attachment 251938 [details]
Async handling
Comment 21 youenn fablet 2015-04-29 03:59:35 PDT
Created attachment 251944 [details]
Rebasing
Comment 22 youenn fablet 2015-04-29 05:25:48 PDT
Comment on attachment 251944 [details]
Rebasing

View in context: https://bugs.webkit.org/attachment.cgi?id=251944&action=review

> Source/WebCore/ChangeLog:8
> +        This patch reads flat file data when DocumentLoader substituteResource delivery timer is fired.

An alternative is to add a Timer at ApplicationCacheHost to do the following process asyncly:
- read flat file
- create SustituteResource using flat file data and appcache resource response
- schedule delivery through DocumentLoader
This is probably more readable code but this requires some additional memory usage/copy.
Hence the current implementation choice based on adding a virtual SusbtituteResource:deliver method, overridden by ApplicationCacheResource..

> Source/WebCore/ChangeLog:9
> +        Refactoring to remove ApplicationCacheHost/DocumentLoader friend link.

This can be put it in another patch if that eases the review.

> Source/WebCore/ChangeLog:10
> +        Added ResourceLoader::deliverResponseAndData helper function, taking a SharedBuffer as input to remove an unneeded copy for flat files (no change for other files). 

See didReceiveData/didReceiveBuffer change in the moved code.

> Source/WebCore/loader/ResourceLoader.cpp:172
> +    // so we need to check if the loader has reached its terminal state.

Since this code is moved within ResourceLoader, we can probably remove these comments here and below.
Comment 23 youenn fablet 2015-05-12 12:16:54 PDT
Except for minor RefPtr/Ref potential refactoring, this patch is probably still valid.
Anyone to review?
Comment 24 Alexey Proskuryakov 2015-05-26 09:54:04 PDT
Comment on attachment 251944 [details]
Rebasing

View in context: https://bugs.webkit.org/attachment.cgi?id=251944&action=review

> Source/WebCore/loader/ResourceLoader.cpp:173
> +    if (reachedTerminalState())

What guarantees that the ResourceLoader object hasn't been destroyed yet?

Even if all current callers have a RefPtr, this function should have it too - it needs the object to stay alive, so it should ensure that.

> Source/WebCore/loader/ResourceLoader.cpp:174
> +

An erroneous blank line here.

> Source/WebCore/loader/ResourceLoader.cpp:178
> +    int size = data->size();

SharedBuffer::size is an unsigned, not an int.

> Source/WebCore/loader/SubstituteResource.h:46
> +    virtual void deliver(ResourceLoader& loader) { loader.deliverResponseAndData(m_response, m_data->copy()); }

Why is copying needed?
Comment 25 youenn fablet 2015-05-26 11:24:06 PDT
Comment on attachment 251944 [details]
Rebasing

View in context: https://bugs.webkit.org/attachment.cgi?id=251944&action=review

>> Source/WebCore/loader/ResourceLoader.cpp:173
>> +    if (reachedTerminalState())
> 
> What guarantees that the ResourceLoader object hasn't been destroyed yet?
> 
> Even if all current callers have a RefPtr, this function should have it too - it needs the object to stay alive, so it should ensure that.

The current caller keeps a RefPtr so there is no risk currently, but you are right it is safer to have a RefPtr<>.
We could do a RefPtr protect.
To remove unnecessary ref++/--, we could also put this code as a static ResourceLoader method taking a RefPtr<>&& as parameter.
I wonder whether that is in line with WebKit style.

>> Source/WebCore/loader/ResourceLoader.cpp:174
>> +
> 
> An erroneous blank line here.

OK

>> Source/WebCore/loader/ResourceLoader.cpp:178
>> +    int size = data->size();
> 
> SharedBuffer::size is an unsigned, not an int.

OK

>> Source/WebCore/loader/SubstituteResource.h:46
>> +    virtual void deliver(ResourceLoader& loader) { loader.deliverResponseAndData(m_response, m_data->copy()); }
> 
> Why is copying needed?

We want SubstituteResource to keep its copy for future loads.
The loader may release or append data to the passed buffer, hence the need to copy it.

In practice, this is often an unnecessary buffer copy.
Splitting append functionality outside SharedBuffer would help solving that.
This might also allow a more maintanable SharedBuffer code.
Comment 26 Alexey Proskuryakov 2015-05-26 11:30:23 PDT
> I wonder whether that is in line with WebKit style.

I haven't seen this done anywhere in WebKit yet.

> The loader may release or append data to the passed buffer, hence the need to copy it.

Ugh.
Comment 27 youenn fablet 2015-05-28 00:20:09 PDT
Created attachment 253838 [details]
Fixing code moved to ResourceLoader
Comment 28 youenn fablet 2015-05-28 03:09:47 PDT
Comment on attachment 253838 [details]
Fixing code moved to ResourceLoader

View in context: https://bugs.webkit.org/attachment.cgi?id=253838&action=review

> Source/WebCore/loader/ResourceLoader.cpp:169
> +    Ref<ResourceLoader> protect(*this);

Following on refcount discussions, current code seems suboptimal here:
- the method calling deliverResponseAndData already incremented the counter
- protect is incrementing the counter inside deliverResponseAndData (just in case)
- didReceiveResponse call (below) will increment/decrement the counter
- didReceiveBuffer sub-call (below) will increment/decrement the counter
- didFinishLoading sub-call (below) inside releaseResources() will increment/decrement the counter

> Source/WebCore/loader/appcache/ApplicationCacheResource.cpp:45
> +        buffer = data()->copy();

copy is returning a PassRefPtr<SharedBuffer> while createWithContentsOfFile is returning a Ref<SharedBuffer>.
Once copy() is refactored to return a Ref<SharedBuffer>, these if/else lines should be refactored.
Plan is to do refactoring around SharedBuffer::copy() as a follow-up patch.
Comment 29 Darin Adler 2015-05-29 15:20:19 PDT
Comment on attachment 253838 [details]
Fixing code moved to ResourceLoader

View in context: https://bugs.webkit.org/attachment.cgi?id=253838&action=review

> Source/WebCore/loader/DocumentLoader.cpp:1170
> +void DocumentLoader::scheduleSubstituteResourceLoad(ResourceLoader* loader, SubstituteResource* resource)

Both arguments should be references, not pointers.

> Source/WebCore/loader/ResourceLoader.cpp:175
> +    RefPtr<SharedBuffer> data = WTF::move(buffer);

No need to move this just to check it for null.

> Source/WebCore/loader/ResourceLoader.cpp:177
> +        unsigned size = data->size();

No need to move this just to get the size.

> Source/WebCore/loader/ResourceLoader.cpp:178
> +        didReceiveBuffer(data.release(), size, DataPayloadWholeResource);

Here is where we should do the move, but actually just calling data.release() should suffice for now. This would turn into a WTF::move if we changed didReceiveBuffer to take a RefPtr&& or Ref&& instead of a PassRefPtr.

> Source/WebCore/loader/SubstituteResource.h:32
> +#include "URL.h"

I think there is no need to include this header if we are also including ResourceResponse.h.

> Source/WebCore/loader/SubstituteResource.h:33
> +#include <wtf/RefCounted.h>

No need to include this header if we are also including URL.h (directly or indirectly).

> Source/WebCore/loader/SubstituteResource.h:34
>  #include <wtf/RefPtr.h>

No need to include this header if we are also including URL.h (directly or indirectly).

> Source/WebCore/loader/appcache/ApplicationCacheResource.h:49
> +    virtual void deliver(ResourceLoader&) override;

I think this can and should be private. Nobody needs to call this non-polymorphically.
Comment 30 youenn fablet 2015-05-31 03:41:40 PDT
Created attachment 253977 [details]
Patch for landing
Comment 31 WebKit Commit Bot 2015-05-31 04:38:28 PDT
Comment on attachment 253977 [details]
Patch for landing

Clearing flags on attachment: 253977

Committed r185040: <http://trac.webkit.org/changeset/185040>
Comment 32 WebKit Commit Bot 2015-05-31 04:38:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Ashley Gullen 2015-05-31 13:42:42 PDT
Great to see this issue finally fixed, thanks to all who contributed!
Comment 34 Ashley Gullen 2015-06-09 05:52:00 PDT
Still reproduces in iOS 9 beta. I guess this was fixed pretty close to release, can I expect the fix to make the next beta?
Comment 35 Ashley Gullen 2015-06-30 08:05:45 PDT
Verified fixed on iOS 9 beta 2. Thanks!