The attached patch removes our offline cache APIs, replacing them with something closer to what has been specified in the webapps spec. The major changes are: * Replace <link rel="offline-resource"> elements with a manifest file that is read and dealt with in nsOfflineCacheUpdate. * Move the scriptable resource list from navigator.offlineResources to window.applicationCache. * Move the update events from navigator.offlineResources to window.applicationCache. * Along the way, make sure the applicationCache object deals correctly with windows that go away and with windows that are frozen in the bfcache. I reused ns[I]DOMOfflineResourceList to implement applicationCache. That should probably be renamed to ns[I]DOMApplicationCache, but I didn't want to dirty up the diff with a bunch of renames. I can do that as a followup. This is not a complete implementation of the whatwg spec. In particular, we are missing versioned, independent application caches and we don't serve from the offline cache unless the browser is explicitly offline.

... and some tests.

Comment on attachment 287152 [details] [diff] [review] v1 - In dom/public/idl/base/nsIDOMClientInformation.idl: - - readonly attribute nsIDOMOfflineResourceList offlineResources; - readonly attribute nsIDOMLoadStatusList pendingOfflineLoads; Do we need to bump the IID here too? Or maybe this never shipped anywhere yet? - In dom/src/offline/nsDOMOfflineResourceList.cpp: +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsDOMOfflineResourceList) + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mManifestURI) + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mDocumentURI) I don't think any URI objects participate in cycle collection, so no real reason to traverse them, or to unlink them in unlink either. - In nsDOMOfflineResourceList::SetOnchecking() etc: + mScriptContext = GetCurrentContext(); This is somewhat concerning, could lead to inconsistent behavior depending on order etc when multiple windows are accessing the same offline resource list. Given that the nsDOMOfflineResourceList knows its window, I'd rather see this code get the context from the window object when it needs it. r+sr=jst with that.

updated with jst's changes. biesi, can you take a look at the nsOfflineCacheUpdate changes please?

Filed bug 405693 about the XXX in nsOfflineCacheManifestItem::OnStopRequest (shouldn't update a byte-for-byte identical cache manifest)

A few small fixes to v2: * The ERROR enum value broke on vc8, so I namespaced the parser states. * Properly fail the update if HandleManifest() indicates that an update is unnecessary.

updated the tests to use ^headers^ instead of modifying httpd.js

Comment on attachment 295171 [details] [diff] [review] v3 +NS_METHOD +nsOfflineManifestItem::ReadManifest(nsIInputStream *aInputStream, please add a /* static */ comment to make it clearer that it's a static member function. + manifest->mReadBuf.Append(aFromSegment + aOffset, aCount); That's not right... see the docs in nsIInputStream. You should start reading at aFromSegment without adding an offset. + nsresult rv = manifest->HandleManifestLine(begin, iter); + NS_ENSURE_SUCCESS(rv, rv); Hm... so if that fails you keep processing the same line over and over again, for each OnDataAvailable call? Perhaps ODA should check that bytesRead == aCount and otherwise return an error. (note: necko requires that onDataAvailable either reads all the data or returns an error (or cancels the request). returning an error from the readSegments callback does not consume the data) + if (!(magic == NS_LITERAL_CSTRING("CACHE MANIFEST"))) { if (!magic.EqualsLiteral("CACHE MANIFEST")) Similar for other such uses in this function +nsOfflineManifestItem::OnStartRequest(nsIRequest *aRequest, I would note that this doesn't check for 404 (or similar), but the MIME type check should be enough. (Otherwise, you could call nsIHttpChannel.requestSucceeded) + nsresult rv = HandleManifestLine(begin, end); (in OnStopRequest) So, here end points beyond the string, so it's not valid to read from it. However, HandleManifestLine does read from it. That's not good. + manifestSpec.SetLength(ref); Should be .Truncate(ref) I think. + if (!mManifestItem) return NS_ERROR_OUT_OF_MEMORY; please put the return on its own line, so that it's possible to set a breakpoint on it.

(In reply to comment #8) > (From update of attachment 295171 [details] [diff] [review]) > > +NS_METHOD > +nsOfflineManifestItem::ReadManifest(nsIInputStream *aInputStream, > > please add a /* static */ comment to make it clearer that it's a static > member function. > done, also on other places > + manifest->mReadBuf.Append(aFromSegment + aOffset, aCount); > > That's not right... see the docs in nsIInputStream. You should start reading > at aFromSegment without adding an offset. fixed as suggested and tested > > + nsresult rv = manifest->HandleManifestLine(begin, iter); > + NS_ENSURE_SUCCESS(rv, rv); > > Hm... so if that fails you keep processing the same line over and over again, > for each OnDataAvailable call? Perhaps ODA should check that bytesRead == > aCount and otherwise return an error. > > (note: necko requires that onDataAvailable either reads all the data or > returns an error (or cancels the request). returning an error from the > readSegments callback does not consume the data) > I set mParserState to PARSER_ERROR and exit reader with NS_ERROR_ABORT, OnDataAvailable then check the parser state and return NS_ERROR_ABORT in case of PARSE_ERROR > + if (!(magic == NS_LITERAL_CSTRING("CACHE MANIFEST"))) { > > if (!magic.EqualsLiteral("CACHE MANIFEST")) > > Similar for other such uses in this function > done > +nsOfflineManifestItem::OnStartRequest(nsIRequest *aRequest, > > I would note that this doesn't check for 404 (or similar), but the MIME type > check should be enough. (Otherwise, you could call > nsIHttpChannel.requestSucceeded) > nsIHttpChannel.requestSucceeded added > + nsresult rv = HandleManifestLine(begin, end); > (in OnStopRequest) > > So, here end points beyond the string, so it's not valid to read > from it. However, HandleManifestLine does read from it. That's not good. > I fixed HandleManifestLine to not to read from the *end position. There were bug in code clearing the trailing spaces and tabs (were not removed because *end points at trailing '\0' char). > + manifestSpec.SetLength(ref); > > Should be .Truncate(ref) I think. > done > + if (!mManifestItem) return NS_ERROR_OUT_OF_MEMORY; > > please put the return on its own line, so that it's possible to set a > breakpoint on it. > done

I just committed the attached patch, which contains honzab's fixes and a change I discussed with biesi on irc (including the nsIOfflineCacheUpdate object in nsIOfflineCacheUpdateObserver methods). Checking in content/base/public/nsContentUtils.h; /cvsroot/mozilla/content/base/public/nsContentUtils.h,v <-- nsContentUtils.h new revision: 1.156; previous revision: 1.155 done Checking in content/base/src/nsContentSink.cpp; /cvsroot/mozilla/content/base/src/nsContentSink.cpp,v <-- nsContentSink.cpp new revision: 1.87; previous revision: 1.86 done Checking in content/base/src/nsContentSink.h; /cvsroot/mozilla/content/base/src/nsContentSink.h,v <-- nsContentSink.h new revision: 1.40; previous revision: 1.39 done Checking in content/base/src/nsContentUtils.cpp; /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v <-- nsContentUtils.cpp new revision: 1.267; previous revision: 1.266 done Checking in content/base/src/nsGkAtomList.h; /cvsroot/mozilla/content/base/src/nsGkAtomList.h,v <-- nsGkAtomList.h new revision: 3.90; previous revision: 3.89 done Checking in content/html/document/src/nsHTMLContentSink.cpp; /cvsroot/mozilla/content/html/document/src/nsHTMLContentSink.cpp,v <-- nsHTMLContentSink.cpp new revision: 3.800; previous revision: 3.799 done Checking in dom/public/base/nsPIDOMWindow.h; /cvsroot/mozilla/dom/public/base/nsPIDOMWindow.h,v <-- nsPIDOMWindow.h new revision: 1.70; previous revision: 1.69 done Checking in dom/public/idl/base/nsIDOMClientInformation.idl; /cvsroot/mozilla/dom/public/idl/base/nsIDOMClientInformation.idl,v <-- nsIDOMClientInformation.idl new revision: 1.5; previous revision: 1.4 done Checking in dom/public/idl/base/nsIDOMWindow2.idl; /cvsroot/mozilla/dom/public/idl/base/nsIDOMWindow2.idl,v <-- nsIDOMWindow2.idl new revision: 1.2; previous revision: 1.1 done Checking in dom/public/idl/offline/nsIDOMOfflineResourceList.idl; /cvsroot/mozilla/dom/public/idl/offline/nsIDOMOfflineResourceList.idl,v <-- nsIDOMOfflineResourceList.idl new revision: 1.2; previous revision: 1.1 done Checking in dom/src/base/nsGlobalWindow.cpp; /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v <-- nsGlobalWindow.cpp new revision: 1.973; previous revision: 1.972 done Checking in dom/src/base/nsGlobalWindow.h; /cvsroot/mozilla/dom/src/base/nsGlobalWindow.h,v <-- nsGlobalWindow.h new revision: 1.320; previous revision: 1.319 done Checking in dom/src/offline/Makefile.in; /cvsroot/mozilla/dom/src/offline/Makefile.in,v <-- Makefile.in new revision: 1.4; previous revision: 1.3 done Removing dom/src/offline/nsDOMOfflineLoadStatusList.h; /cvsroot/mozilla/dom/src/offline/nsDOMOfflineLoadStatusList.h,v <-- nsDOMOfflineLoadStatusList.h new revision: delete; previous revision: 1.3 done Checking in dom/src/offline/nsDOMOfflineResourceList.cpp; /cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.cpp,v <-- nsDOMOfflineResourceList.cpp new revision: 1.5; previous revision: 1.4 done Checking in dom/src/offline/nsDOMOfflineResourceList.h; /cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.h,v <-- nsDOMOfflineResourceList.h new revision: 1.3; previous revision: 1.2 done Checking in dom/tests/mochitest/ajax/offline/Makefile.in; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/Makefile.in,v <-- Makefile.in new revision: 1.6; previous revision: 1.5 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest,v done Checking in dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest,v <-- badManifestMagic.cacheManifest initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest^headers^,v done Checking in dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest^headers^; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/badManifestMagic.cacheManifest^headers^,v <-- badManifestMagic.cacheManifest^headers^ initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest,v done Checking in dom/tests/mochitest/ajax/offline/missingFile.cacheManifest; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest,v <-- missingFile.cacheManifest initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest^headers^,v done Checking in dom/tests/mochitest/ajax/offline/missingFile.cacheManifest^headers^; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/missingFile.cacheManifest^headers^,v <-- missingFile.cacheManifest^headers^ initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineChild.html,v done Checking in dom/tests/mochitest/ajax/offline/offlineChild.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineChild.html,v <-- offlineChild.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineTests.js,v done Checking in dom/tests/mochitest/ajax/offline/offlineTests.js; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineTests.js,v <-- offlineTests.js initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest,v done Checking in dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest,v <-- simpleManifest.cacheManifest initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest^headers^,v done Checking in dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest^headers^; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.cacheManifest^headers^,v <-- simpleManifest.cacheManifest^headers^ initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.notmanifest,v done Checking in dom/tests/mochitest/ajax/offline/simpleManifest.notmanifest; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/simpleManifest.notmanifest,v <-- simpleManifest.notmanifest initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMagic.html,v done Checking in dom/tests/mochitest/ajax/offline/test_badManifestMagic.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMagic.html,v <-- test_badManifestMagic.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMime.html,v done Checking in dom/tests/mochitest/ajax/offline/test_badManifestMime.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_badManifestMime.html,v <-- test_badManifestMime.html initial revision: 1.1 done Removing dom/tests/mochitest/ajax/offline/test_isLocallyAvailable.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_isLocallyAvailable.html,v <-- test_isLocallyAvailable.html new revision: delete; previous revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_missingFile.html,v done Checking in dom/tests/mochitest/ajax/offline/test_missingFile.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_missingFile.html,v <-- test_missingFile.html initial revision: 1.1 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_offlineIFrame.html,v done Checking in dom/tests/mochitest/ajax/offline/test_offlineIFrame.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_offlineIFrame.html,v <-- test_offlineIFrame.html initial revision: 1.1 done Removing dom/tests/mochitest/ajax/offline/test_offlineResources.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_offlineResources.html,v <-- test_offlineResources.html new revision: delete; previous revision: 1.1 done Removing dom/tests/mochitest/ajax/offline/test_pendingOfflineLoads.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_pendingOfflineLoads.html,v <-- test_pendingOfflineLoads.html new revision: delete; previous revision: 1.2 done RCS file: /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_simpleManifest.html,v done Checking in dom/tests/mochitest/ajax/offline/test_simpleManifest.html; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/test_simpleManifest.html,v <-- test_simpleManifest.html initial revision: 1.1 done Checking in uriloader/prefetch/nsIOfflineCacheUpdate.idl; /cvsroot/mozilla/uriloader/prefetch/nsIOfflineCacheUpdate.idl,v <-- nsIOfflineCacheUpdate.idl new revision: 1.2; previous revision: 1.1 done Checking in uriloader/prefetch/nsOfflineCacheUpdate.cpp; /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v <-- nsOfflineCacheUpdate.cpp new revision: 1.5; previous revision: 1.4 done Checking in uriloader/prefetch/nsOfflineCacheUpdate.h; /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.h,v <-- nsOfflineCacheUpdate.h new revision: 1.4; previous revision: 1.3 done

Comment on attachment 297417 [details] [diff] [review] patch as committed Just a note that this patch is probably not the one that got into CVS. It for example doesn't contain my fix for the trailing spaces remove in nsOfflineManifestItem::HandleManifestLine.

It seems like the patch that fixed this had tests, so I'm marking this in-testsuite+. Or are more tests needed for this bug? Then please set the in-testsuite flag back to ?

This is now documented: http://developer.mozilla.org/en/docs/Offline_resources_in_Firefox http://developer.mozilla.org/en/docs/nsIDOMOfflineResourceList