It's a waste of time to read through from already cached or local content. This patch will make blob response much faster regardless of the response size.

I'd really like to see tests for this. For example, will this fire a "progress" event with the proper values for .loaded/.total/.lengthComputable? And will "load" and "loadend" get the proper values for those properties? You can probably add to the test_xhr_progressevents.html test to add a load of a large cached resource when .responseType is "blob".

(In reply to Jonas Sicking (:sicking) from comment #1) If I send the second consecutive sync request to the same resource, sometimes the request fails to open the cache because the cache is "busy". If I send the async request, callback will not be fired until the first response is GCed (it took 15 minutes on my test). If I hold the reference to the first response, the callback will never be fired. This bug (?) seems to be unrelated to the patch. Is there a reliable way to ensure the resource is cached without disturbing the second request? Or should I investigate this bug?

CC'ing Jason for help on the cache issue. You say "If I hold the reference to the first response, the callback will never be fired". What callback are you referring to? And is the fact that it doesn't fire the bug you are referring to in the following sentence?

This could be the issue we have where when an initial channel creates and opens a cache entry for writing, the 2nd and following requests to that URI will block until the writer releases the cache entry. In a regular, successful request that's done in OnStopRequest. I'm not clear on why it's not happening if you hang onto the XHR request itself. But if the underlying nsHTTPChannel's mCacheEntry is not null for the first request, it'll block other channels from reading the cache entry indefinitely.

> // We don't have to read from the local file for the blob response > return request->Cancel(NS_OK); Aha--I finally encounter a case where channel->Cancel(NS_OK) makes sense. I've been wondering why we support that.

Though if you were wanting to remove that, we can easily work around it.

Comment on attachment 562273 [details] [diff] [review] patch Canceling review until the test problem is resolved.

I think I found a cause.

Comment on attachment 562876 [details] [diff] [review] Part 2 - Fix the chache issue Grr, it caused many necko test failures.

Marking valid only when the cache is stored on the disk file.

Comment on attachment 563220 [details] [diff] [review] Part 2 - Fix the cache issue It looks to be passed the try, so requesting review.

Comment on attachment 563220 [details] [diff] [review] Part 2 - Fix the cache issue Review of attachment 563220 [details] [diff] [review]: ----------------------------------------------------------------- Michal, can you take this (or pass to bjarne/nick or someone else working on the cache)? Needed for newest version of XMLHttpRequest to work.

rebased to tip

rebased to tip

Comment on attachment 563552 [details] [diff] [review] Part 1 - Skip reading already cached or local resource for blob response Review of attachment 563552 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsXMLHttpRequest.cpp @@ +1701,5 @@ > if (fc) { > fc->GetFile(getter_AddRefs(file)); > } > } > + bool fromFile = false; Seems cleaner to move the logic for setting fromFile to the if-statement above. Can GetCacheFile really return a file while IsFromCache returns false? I.e. can fromFile ever be false when file is true?

Comment on attachment 562878 [details] [diff] [review] Part 3 - Test Review of attachment 562878 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/test/test_xhr_progressevents.html @@ +152,5 @@ > { data: utf8encode("a\u867Eb").substr(3,1), utf16: "\u867E" }, > { data: utf8encode("a\u867Eb").substr(4), utf16: "b" }, > { close: true }, > + { file: "file_XHR_binary2.bin", name: "cacheable data", total: 65536 }, > + { file: "file_XHR_binary2.bin", name: "cached data", total: 65536 }, Split these into two entries each, like: { file: ... }, { close: true }, That way you don't have to duplicate the loop which waits for data to come in. @@ +160,5 @@ > for (let i = 0; i < tests.length; ++i) { > let test = tests[i]; > + if (responseType.type !== "blob" && "file" in test) { > + continue; > + } Instead of doing it this way, add something like: if (responseType.type === "blob") { tests.push({ file: ... }, { file: ... }); } just after the tests array is created, outside the inner loop. @@ +175,5 @@ > encoded: test.encoded, > nodata: responseType.nodata, > chunked: responseType.chunked, > + text: responseType.text, > + cacheable: "file" in test }; I'd just do: file: test.file That way the file-specific behavior is more specific. We might want to add tests for cachable non-file resources later. @@ +180,4 @@ > > xhr.onreadystatechange = null; > + if (testState.cacheable) > + xhr.open("GET", test.file); Then you can use testState.file in both these locations. @@ +199,5 @@ > } > + if ("file" in test) { > + testState.pendingBytes = testState.total; > + testState.pendingResult = fileExpectedResult; > + while(testState.pendingBytes) { By breaking out the 'close' entry as described above, you can remove this while-loop.

Comment on attachment 563552 [details] [diff] [review] Part 1 - Skip reading already cached or local resource for blob response Review of attachment 563552 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed.

(In reply to Jonas Sicking (:sicking) from comment #17) > Seems cleaner to move the logic for setting fromFile to the if-statement > above. Can GetCacheFile really return a file while IsFromCache returns > false? I.e. can fromFile ever be false when file is true? GetCacheFile will return the partial or empty file when the response is going be cached. In this case, file will be non-null and fromFile will be false. We should not skip reading from the network unless the response is fully cached. Otherwise, we will return the broken blob response to the content.

Added a comment to clarify the intention.

Resolved review comments.

Comment on attachment 566231 [details] [diff] [review] Part 3 - Tests, v2 Review of attachment 566231 [details] [diff] [review]: ----------------------------------------------------------------- r=me

Michal, ping?

Comment on attachment 563553 [details] [diff] [review] Part 2 - Fix the cache issue > + NS_SUCCEEDED(GetCacheAsFile(&asFile)) && asFile) { Why is this limited to entries that are stored as file? Otherwise looks good. And sorry for the delay, the request for review got lost in my mailbox...

(In reply to Michal Novotny (:michal) from comment #25) > Comment on attachment 563553 [details] [diff] [review] [diff] [details] [review] > Part 2 - Fix the cache issue > > > + NS_SUCCEEDED(GetCacheAsFile(&asFile)) && asFile) { > > Why is this limited to entries that are stored as file? > > Otherwise looks good. And sorry for the delay, the request for review got > lost in my mailbox... This patch causes test failures if this check is removed (see comment #11). Do I have to investigate further?

(In reply to Masatoshi Kimura [:emk] from comment #26) > This patch causes test failures if this check is removed (see comment #11). > Do I have to investigate further? Ah, I think I know what causes the failures. We write to the cache asynchronously when it isn't stored in the file and it isn't completely written to the disk yet in nsHttpChannel::OnStopRequest().

Comment on attachment 563553 [details] [diff] [review] Part 2 - Fix the cache issue Maybe add the information from comment #27 to the comment in the source file.

Thank you. I updated a comment. Transferring r+.

Part 1 fails to apply. I didn't try the others. By the way, your commit messages lack the review part.

Ah, I've forgotten to attach an unbitrotted version for PRBool->bool migration.

> By the way, your commit messages lack the review part. Sorry, I don't understand what does it mean. Here are commit messages from the patch files. > Bug 689008: Part 1 - Skip reading already cached or local resource for blob response > Bug 689008: Part 2 - Fix the cache issue > Bug 689008: Part 3 - Test

Ah, I got it. Added reviewer names.

Has this been sent to try? If not, I'll do so before pushing, once the try situation is resolved (bug 696682).

Try run for 05ad4052ca88 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=05ad4052ca88 Results (out of 190 total builds): success: 183 warnings: 7 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/VYV03354@nifty.ne.jp-05ad4052ca88

Pushed: http://hg.mozilla.org/integration/mozilla-inbound/rev/5db5141fb595 http://hg.mozilla.org/integration/mozilla-inbound/rev/e050ed22381c http://hg.mozilla.org/integration/mozilla-inbound/rev/254bfb46c100

https://hg.mozilla.org/mozilla-central/rev/5db5141fb595 https://hg.mozilla.org/mozilla-central/rev/e050ed22381c https://hg.mozilla.org/mozilla-central/rev/254bfb46c100