Right now the offline cache's owner domain is always the hostPort of the URI that added the item to the cache. This was done originally because navigator.offlineResources was scoped to the hostport. At this point the owner is only used during usage calculations and when removing an offline app. The hostport is not really safe to persist like that - bug 400097 details a similar problem with the permission manager. Now that we scope applications by a manifest URI rather than a hostport, and we manage access to the offline cache with the permission manager (which is host only), we should change users of the offline cache to use asciiHost as the owner rather than hostPort. This should be a pretty easy fix.

Comment on attachment 293918 [details] [diff] [review] Changing hostport to asciihost on appropriate places Dcamp should review this, I'll happily sr once reviewed...

Comment on attachment 293918 [details] [diff] [review] Changing hostport to asciihost on appropriate places >--- dom/tests/mochitest/ajax/offline/offlineTests.js 2007-12-07 18:55:20.347630100 +0100 >+++ dom/tests/mochitest/ajax/offline/offlineTests.js 2007-12-19 22:14:22.788203200 +0100 >@@ -107,25 +107,25 @@ clear: function() > // Clear manifest-owned urls >- cacheSession.setOwnedKeys(window.location.host, >+ cacheSession.setOwnedKeys(window.location.hostname, > this.getManifestUrl() + "#manifest", 0, []); > > // Clear dynamically-owned urls >- cacheSession.setOwnedKeys(window.location.host, >+ cacheSession.setOwnedKeys(window.location.hostname, > this.getManifestUrl() + "#dynamic", 0, []); > > cacheSession.evictUnownedEntries(); These should probably be window.location.asciiHost. Looks ok other than that.

(In reply to comment #3) > These should probably be window.location.asciiHost. That won't work, window.location is an nsLocation, not a nsIURI. I'm not sure there's any easy way to get the asciiHost... I suppose you'd have to get the URI from the docshell yourself.

(In reply to comment #4) > (In reply to comment #3) > > These should probably be window.location.asciiHost. > > That won't work, window.location is an nsLocation, not a nsIURI. I'm not sure > there's any easy way to get the asciiHost... I suppose you'd have to get the > URI from the docshell yourself. Ah right. Well this is just a test case using simple hostnames, so the original patch should be fine.

Agree. See http://developer.mozilla.org/en/docs/DOM:window.location

Added missing change in the header file

Comment on attachment 295839 [details] [diff] [review] Changing hostport to asciihost on appropriate places, v2 dom/tests/mochitest/ajax/offline/offlineTests.js: - cacheSession.setOwnedKeys(window.location.host, + cacheSession.setOwnedKeys(window.location.hostname, this.getManifestUrl() + "#manifest", 0, []); While I realize this is in a test, the API here seems bad if this is how people are expected to use it. location.hostname comes from nsIURI::GetHost()'s UTF8 result being converted to UTF16, and here it's being passed to a method taking an ACString argument, which means we'll do a lossy UTF16 to ASCII conversion and we'll potentially set the wrong keys here. An easy way to fix this would be to change the type of the arguments to setOwnedKeys to take AUTF8String arguments instead of ACString arguments (binary compatible change), which would mean XPConnect does a correct UTF16 to UTF8 conversion for users of this API. Do we want that, or does it not matter for some reason?

We want the ownerHost to be an ascii-ified domain name, so I think nsACString& is appropriate. Consumers of the API should really be using an nsIURI's asciiHost. We could update the test to create an nsIURI from window.location. That would get it right, and provide a better example. We *do* need to either update the spec argument to take an AUTF8String, or update the patch to send asciiSpec for that too. I'd probably argue the latter, for the same reasons we're making the asciiHost change.

Comment on attachment 295839 [details] [diff] [review] Changing hostport to asciihost on appropriate places, v2 r- for the asciiSpec issue.

Test is loading the location.href to standard URL spec and then uses its asciiHost attribute to pass as domain owner.

I changed comments and names of the parameters in nsIOfflineCacheSession.idl according to discussion with Dave Camp. Also changing use of nsIURI.spec attribute to asciiSpec on several places: 1. In nsOfflineCacheUpdate - the #manifest and #dynamic keys were changed to use asciiSpec base 2. nsDOMOfflineResourceList class - same as point 1. for dynamic key 3. nsDOMOfflineResourceList::Remove method - taking nsIURI parameter and removing it from the cache as a key uses internal method GetCacheKey that was extracting spec from that URI -> changed to use asciiSpec because keys are also just ASCII encoded

Comment on attachment 298295 [details] [diff] [review] Changing hostport to asciihost on appropriate places, v4 rs=biesi

Checking in dom/src/offline/nsDOMOfflineResourceList.cpp; /cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.cpp,v <-- nsDOMOfflineResourceList.cpp new revision: 1.6; previous revision: 1.5 done Checking in dom/src/offline/nsDOMOfflineResourceList.h; /cvsroot/mozilla/dom/src/offline/nsDOMOfflineResourceList.h,v <-- nsDOMOfflineResourceList.h new revision: 1.4; previous revision: 1.3 done Checking in dom/tests/mochitest/ajax/offline/offlineTests.js; /cvsroot/mozilla/dom/tests/mochitest/ajax/offline/offlineTests.js,v <-- offlineTests.js new revision: 1.2; previous revision: 1.1 done Checking in netwerk/cache/public/nsIOfflineCacheSession.idl; /cvsroot/mozilla/netwerk/cache/public/nsIOfflineCacheSession.idl,v <-- nsIOfflineCacheSession.idl new revision: 1.4; previous revision: 1.3 done Checking in uriloader/prefetch/nsOfflineCacheUpdate.cpp; /cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v <-- nsOfflineCacheUpdate.cpp new revision: 1.6; previous revision: 1.5 done

Does the offlineTests.js change test this adequately even if we're only running tests on domains where asciiHost is no different from window.location.host? It shouldn't be hard to add such a domain to Mochitest (it's going to happen any day now for another bug) and have a test that would exercise this.