Closed Bug 1168300 Opened 9 years ago Closed 9 years ago

Add cookieJar attribute and notify clear-cookiejar-data

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 9 obsolete files)

In Bug 1163254 we will add new attributes for origin.
Right now mozIApplicationClearPrivateDataParams uses 'appId' and 'browserOnly', we should fix this.
Component: DOM → DOM: Apps
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
Flags: needinfo?(bobbyholley)
(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?
Flags: needinfo?(bobbyholley) → needinfo?(jonas)
(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.
Flags: needinfo?(jonas)
Summary: Use origin attribute in mozIApplicationClearPrivateDataParams → Use cookieJar attribute in mozIApplicationClearPrivateDataParams
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
Summary: Use cookieJar attribute in mozIApplicationClearPrivateDataParams → add cookieJar attribute in mozIApplicationClearPrivateDataParams
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
Summary: add cookieJar attribute in mozIApplicationClearPrivateDataParams → use cookieJar attribute to notify webapps-clear-cookiejar-data
Summary: use cookieJar attribute to notify webapps-clear-cookiejar-data → Add cookieJar attribute and notify webapps-clear-cookiejar-data
Attached patch WIP Patch (obsolete) — Splinter Review
I am still trying to add a test for a brower frame.
Attached patch WIP Patch v2. (obsolete) — Splinter Review
Attachment #8612238 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #8612716 - Attachment is obsolete: true
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
Attachment #8612791 - Flags: review?(jonas)
Attachment #8612791 - Flags: feedback?(bobbyholley)
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).
Attachment #8612791 - Flags: feedback?(bobbyholley) → feedback+
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.
Attached patch Patch v2. (obsolete) — Splinter Review
rebase on Bug 1170097 to user helper from ChromeUtils and remove mozIApplicationClearCookieJarParams
Attachment #8612791 - Attachment is obsolete: true
Attachment #8612791 - Flags: review?(jonas)
Attachment #8613443 - Flags: review?(jonas)
Attached patch Patch v2. (obsolete) — Splinter Review
qref
Attachment #8613443 - Attachment is obsolete: true
Attachment #8613443 - Flags: review?(jonas)
Attached patch interdiff (v1 - v2) (obsolete) — Splinter Review
Attachment #8613444 - Attachment is obsolete: true
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.
Attachment #8613449 - Flags: feedback?(bobbyholley)
(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
Attachment #8613449 - Flags: feedback?(bobbyholley)
Attached patch Patch. v3 (obsolete) — Splinter Review
Attachment #8613447 - Attachment is obsolete: true
Attachment #8613449 - Attachment is obsolete: true
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.
Attachment #8613463 - Flags: review?(jonas) → review+
Attached patch Patch. v4 (obsolete) — Splinter Review
addressed jonas' comments.
Attachment #8613463 - Attachment is obsolete: true
Attachment #8616544 - Flags: review+
Summary: Add cookieJar attribute and notify webapps-clear-cookiejar-data → Add cookieJar attribute and notify clear-cookiejar-data
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
Flags: needinfo?(allstars.chh)
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
Attached patch Patch v5.Splinter Review
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
Attachment #8616544 - Attachment is obsolete: true
Flags: needinfo?(allstars.chh)
Attachment #8624009 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8ae5db65a77a
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
No longer depends on: 1163254
Blocks: 1179985
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: