Bug 138506

Still reproduces on iOS 8.2!

Still reproduces on iOS 8.3. Please fix it! It stops anything using Web Audio from working offline!

(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.

Created attachment 250509 [details] First stab

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

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 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

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 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 .

Created attachment 250520 [details] Fixing tests

Created attachment 250597 [details] Better integration

Created attachment 250600 [details] Trying to fix iOS build

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.

(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.

> 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.

(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?

Created attachment 250668 [details] WIP using platformStrategies

(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.

Created attachment 250703 [details] Patch

Created attachment 251938 [details] Async handling

Created attachment 251944 [details] Rebasing

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.

Except for minor RefPtr/Ref potential refactoring, this patch is probably still valid. Anyone to review?

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 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.

> 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.

Created attachment 253838 [details] Fixing code moved to ResourceLoader

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 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.

Created attachment 253977 [details] Patch for landing

Comment on attachment 253977 [details] Patch for landing Clearing flags on attachment: 253977 Committed r185040: <http://trac.webkit.org/changeset/185040>

All reviewed patches have been landed. Closing bug.

Great to see this issue finally fixed, thanks to all who contributed!

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?

Verified fixed on iOS 9 beta 2. Thanks!