402272 - move offline cache APIs toward the whatwg spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Dave Camp (:dcamp)]

Attachment: v1

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.

Comment 1

[Dave Camp (:dcamp)]

Attachment: tests

... and some tests.

Comment 3

[Johnny Stenback (:jst)]

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.

Comment 4

[Dave Camp (:dcamp)]

Attachment: v2

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

Comment 5

[Dave Camp (:dcamp)]

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

Comment 6

[Dave Camp (:dcamp)]

Attachment: v3

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.

Comment 7

[Dave Camp (:dcamp)]

Attachment: tests v2

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

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v4

(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

Comment 10

[Dave Camp (:dcamp)]

Attachment: patch as committed

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 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Martijn Wargers (dead)]

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 ?

Comment 13

[Eric Shepherd [:sheppy]]

This is now documented:

http://developer.mozilla.org/en/docs/Offline_resources_in_Firefox
http://developer.mozilla.org/en/docs/nsIDOMOfflineResourceList