In Bug 1163254 we will add new attributes for origin. Right now mozIApplicationClearPrivateDataParams uses 'appId' and 'browserOnly', we should fix this.

Hi Bobby Do you think it makes sense that to add a helper to convert originAttribute(jsval) to originSuffix(ACString) ? My idea is to covert/change mozIApplicationClearPrivateDataParams to OriginAttributesDictionary. So when observing webapps-clear-data, the observers could convert the jsval to a string(with the same format with originSuffix), then query the origins by originSuffix. If so, where should I put those helpers? or feel free to comment if you have other idea. Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #1) > Hi Bobby > Do you think it makes sense that to add a helper to convert > originAttribute(jsval) to originSuffix(ACString) ? > > My idea is to covert/change mozIApplicationClearPrivateDataParams to > OriginAttributesDictionary. > So when observing webapps-clear-data, the observers could convert the jsval > to a string(with the same format with originSuffix), then query the origins > by originSuffix. Hey Yoshi, My understanding is that this is the exact situation that Jonas wanted to use .cookieJar for. Rather than passing around mozIApplicationClearPrivateDataParams, we'd just pass a string (the opaque cookie jar), and the consumers (like QuotaManager::ClearStoragesForApp) would need to know how to clear data for a particular cookie jar. That would mean that the QuotaManager may need to change its internal storage representation to store cookieJars, rather than the 'OriginPattern' that it appears to use now. Jonas, did I get that right?

(In reply to Bobby Holley (:bholley) from comment #2) > My understanding is that this is the exact situation that Jonas wanted to > use .cookieJar for. Rather than passing around > mozIApplicationClearPrivateDataParams, we'd just pass a string (the opaque > cookie jar), and the consumers (like QuotaManager::ClearStoragesForApp) > would need to know how to clear data for a particular cookie jar. That would > mean that the QuotaManager may need to change its internal storage > representation to store cookieJars, rather than the 'OriginPattern' that it > appears to use now. > > Jonas, did I get that right? Bobby, I also agree on this except I plan to use originSuffix/originAttribute instead of cookieJar. The problem I have in mind is right now in Webapps.jsm, when it dispatches webapps-clear-data event, it looks to me that it doesn't have the information of the principal for the deleted app (mozIApplication). But the details should be done in this bug and I'll try to figure this out. But I think it's easier to do this as two steps, the first step will be replace to {appId, inBrowser} with originAttribute, since the callers already know {appId, inBrowser}. But on the other hand those callers should use the 'origin' you've done, the {origin}!appId=123&inBrowser=1 one. So my question to you is should we have some helper that coverts {appId, inBrowser} i.e. originAttribute dict, to the opaque string (originSuffix), since the opaque string should be an internal format, those callers shouldn't do the + '!appId=' + appId + '&inBrowser='+inBrowser; by themself. Is my understanding correct? And the 2nd step is to replace the originAttibute to the opaque string, as I mentioned earlier I'll look into DOM:Apps to figure out how to get principal from the app. Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #3) > Bobby, I also agree on this except I plan to use > originSuffix/originAttribute instead of cookieJar. I think that Jonas specifically wants to use cookieJar here (though we can let him confirm). Remember, we want to use origin + originAttributes for everything _except_ the key/tag/identifier for clearing private data (See https://hg.mozilla.org/mozilla-central/annotate/e537a1ba501b/caps/nsIPrincipal.idl#l207 ). If I understand correctly, mozIApplicationClearPrivateDataParams is precisely this case. > The problem I have in mind is right now in Webapps.jsm, when it dispatches > webapps-clear-data event, it looks to me that it doesn't have the > information of the principal for the deleted app (mozIApplication). But the > details should be done in this bug and I'll try to figure this out. We can always create such a principal |ssm.createCodebasePrincipal(originURI, attrs)|, and then pull any data that we need off of it. I agree that it would be better for mozIApplication to expose a principal. > But I think it's easier to do this as two steps, > the first step will be replace to {appId, inBrowser} with originAttribute, > since the callers already know {appId, inBrowser}. > But on the other hand those callers should use the 'origin' you've done, the > {origin}!appId=123&inBrowser=1 one. For most situations I agree, but per above I think we actually want cookieJar here instead. > So my question to you is should we have some helper that coverts {appId, > inBrowser} i.e. originAttribute dict, to the opaque string (originSuffix), > since the opaque string should be an internal format, those callers > shouldn't do the + '!appId=' + appId + '&inBrowser='+inBrowser; by themself. The way to do this is: ssm.createCodebasePrincipal(dummyURI, {...}).originSuffix(). Does that make sense?

Bobby explained to me on irc.

As I found out there are lots of callers of mozIApplicationClearPrivateDataParams, so my plan is 1. Add cookieJar in mozIApplicationClearPrivateDataParams, while still keeping appId and browserOnly to prevent breaking things. 2. Went back to fix up the bugs depending Bug 1163254. 3. Hopefully at that time there's no more consumers of mozIApplicationClearPrivateDataParams.{appId|browserOnly} Then I'd file another bug to remove {appId|browserOnly} from mozIApplicationClearPrivateDataParams. Below is the list of callers of mozIApplicationClearPrivateDataParams. dom/push/PushService.jsm dom/quota/QuotaManager.cpp dom/messages/SystemMessageInternal.js dom/requestsync/RequestSyncService.jsm dom/storage/DOMStorageObserver.cpp dom/simplepush/PushService.jsm dom/datastore/DataStoreService.cpp dom/alarm/AlarmService.jsm dom/apps/InterAppCommService.jsm extensions/cookie/test/unit/test_permmanager_cleardata.js extensions/cookie/nsPermissionManager.cpp netwerk/protocol/http/nsHttpAuthCache.cpp netwerk/test/unit/test_cache_jar.js netwerk/test/unit/test_auth_jar.js netwerk/cache2/CacheObserver.cpp

Sounds good.

One thing that's tricky with the current "webapps-clear-data" notification is that it sends a single notification which sometimes clears one cookiejar and sometimes clears two cookiejars. When "browserOnly" is set to true, it only clears data with appid = mozIACPDP.appid [1] and the inBrowser = true. But when "browserOnly" is false, it clears all data with appid = mozIACPDP.appid and inBrowser = true as well as appid = mozIACPDP.appid and inBrowser = false. I.e. effectively two cookiejars are cleared. But there's no way to express this through a .cookieJar property without inventing some sort of wildcard syntax, which I think would be too complicated. What I think we should do is to add a new observerService notification called "clear-cookiejar-data" which uses the cookiejar as the third argument to the observerService. Then anytime that we send the "webapps-clear-data" notification, we'll send one or two "clear-cookiejar-data" notifications depending on what we need. Then we can migrate code from using "webapps-clear-data" to using "clear-cookiejar-data", and in the process stop using appid/inbrowser and start using cookiejar. [1] mozIACPDP = mozIApplicationClearPrivateDataParams

update bug title per Jonas' comment in Comment 8

I am still trying to add a test for a brower frame.

I still couldn't make clearBrowserData() working in mochitest :( I try to do something similar from ./dom/tests/mochitest/localstorage/test_clear_browser_data.html however mozApps.getSelf() always returns null (I guess that means the context of the test html still in browser mode) I'll file another bug to finish the test.

Comment on attachment 8612791 [details] [diff] [review] Patch Review of attachment 8612791 [details] [diff] [review]: ----------------------------------------------------------------- r? to Jonas as he reviewed the original code from Bug 794563. and feedback? to bholley for the usage of CAPS interface. For now I create another mozIApplicationClearCookieJarParams for the cookieJar attribute, but if you think adding 3rd paramater in mozIApplicationClearPrivateDataParams is fine I'll update it. Thanks try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48560e45dd88

Comment on attachment 8612791 [details] [diff] [review] Patch Review of attachment 8612791 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/Webapps.jsm @@ +4867,5 @@ > + > + _clearCookieJarData: function(appId, browserOnly, msg) { > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > + let dummy = Services.io.newURI('http://dummy.com', null, null);; Use example.com for such things. This is a decent hack, but I think we should add a chrome-only WebIDL method (on ChromeUtils.webidl) called originAttributesToCookieJar, which does this work. We could also add originAttributesToSuffix while we're at it. ::: dom/interfaces/apps/mozIApplicationClearPrivateDataParams.idl @@ +16,5 @@ > > +[scriptable, uuid(43975b8c-478b-46f1-96e8-70b22a3370f6)] > +interface mozIApplicationClearCookieJarParams : nsISupports > +{ > + readonly attribute ACString cookieJar; Why do we need this interface? Given that the whole point of cookieJar is that we have a single key for everything, it seems like we should just pass the string directly (possibly as a variant). This should be pretty easy to do - let me know if you're not sure how (I could do it, if you like).

Comment on attachment 8612791 [details] [diff] [review] Patch Review of attachment 8612791 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/Webapps.jsm @@ +4867,5 @@ > + > + _clearCookieJarData: function(appId, browserOnly, msg) { > + let secMan = Cc["@mozilla.org/scriptsecuritymanager;1"] > + .getService(Ci.nsIScriptSecurityManager); > + let dummy = Services.io.newURI('http://dummy.com', null, null);; Filed Bug 1170097 for originAttributesToCookieJar ::: dom/interfaces/apps/mozIApplicationClearPrivateDataParams.idl @@ +16,5 @@ > > +[scriptable, uuid(43975b8c-478b-46f1-96e8-70b22a3370f6)] > +interface mozIApplicationClearCookieJarParams : nsISupports > +{ > + readonly attribute ACString cookieJar; I am glad to do it, but I am not sure if I fully understand this. Do you mean I should use wrappedJSObject, and remove mozIApplicationClearCookieJarParams? I'll write a new version for this and f? you again.

rebase on Bug 1170097 to user helper from ChromeUtils and remove mozIApplicationClearCookieJarParams

qref

You don't need an object at all for the new notification. Look at http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService.idl#61 The second argument is the name of the notification. I.e. "webapps-clear-cookiejar-data". The first and the third argument is arbitrary information which is passed to all observers. Since we just need to pass a string to all observers just use the third argument.

(In reply to Jonas Sicking (:sicking) from comment #20) > You don't need an object at all for the new notification. Look at > > http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService. > idl#61 > > The second argument is the name of the notification. I.e. > "webapps-clear-cookiejar-data". > > The first and the third argument is arbitrary information which is passed to > all observers. Since we just need to pass a string to all observers just use > the third argument. Ah, I see. Thanks

Comment on attachment 8613463 [details] [diff] [review] Patch. v3 Review of attachment 8613463 [details] [diff] [review]: ----------------------------------------------------------------- Also remove the 'msg' argument, at least from this new function, if it's not needed. ::: dom/apps/Webapps.jsm @@ +4868,5 @@ > + _clearCookieJarData: function(appId, browserOnly, msg) { > + let browserCookieJar = > + ChromeUtils.originAttributesToCookieJar({appId: appId, > + inBrowser: true}); > + this._notifyCategoryAndObservers(null, "webapps-clear-cookiejar-data", I think we should simply call the notification "clear-cookiejar-data". This notification won't actually be webapps specific but rather something that we'll use anytime we'll clear a cookiejar. It just happens that when we uninstall an app, we'll clear two cookiejars. But in the future other things will clear cookiejars too.

addressed jonas' comments.

I noticed one try error in 224 INFO TEST-UNEXPECTED-FAIL | dom/apps/tests/test_bug_1168300.html | Test timed out. - expected PASS 06-11 09:06:05.313 F/libc ( 805): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1) This usually indicates the B2G process has crashed 821420 Intermittent crash during mochitests on B2G ("This usually indicates the B2G process has crashed" after a " | application timed out after 330 seconds with no output") from https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a0af4ff3aa I didn't meet this problem in my local build, but it still occurs feel free to back out my patch. :D

(In reply to Yoshi Huang[:allstars.chh] from comment #26) > I didn't meet this problem in my local build, but it still occurs feel free but if it > to back out my patch. :D

hey Yoshi, sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2139567&repo=b2g-inbound

Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/3feb2f31cec7

The newly-added test was failing intermittently on b2g emulator builds as well. https://treeherder.mozilla.org/logviewer.html#?job_id=2140316&repo=b2g-inbound

update test case to fix try failure on other platform. Also skip the test on ICS, as I'll fix it in Bug 1175784. try https://treeherder.mozilla.org/#/jobs?repo=try&revision=15a11eec21a7