Add cookieJar attribute and notify clear-cookiejar-data

RESOLVED FIXED in Firefox 41

Status

Core Graveyard
DOM: Apps
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: allstars, Assigned: allstars)

Tracking

unspecified
mozilla41
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

3 years ago
In Bug 1163254 we will add new attributes for origin.
Right now mozIApplicationClearPrivateDataParams uses 'appId' and 'browserOnly', we should fix this.
(Assignee)

Updated

3 years ago
Blocks: 1165787
(Assignee)

Updated

3 years ago
Component: DOM → DOM: Apps
(Assignee)

Comment 1

3 years ago
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)
(Assignee)

Comment 3

3 years ago
(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?
(Assignee)

Comment 5

3 years ago
Bobby explained to me on irc.
Flags: needinfo?(jonas)
(Assignee)

Updated

3 years ago
Summary: Use origin attribute in mozIApplicationClearPrivateDataParams → Use cookieJar attribute in mozIApplicationClearPrivateDataParams
(Assignee)

Comment 6

3 years ago
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
(Assignee)

Updated

3 years ago
Summary: Use cookieJar attribute in mozIApplicationClearPrivateDataParams → add cookieJar attribute in mozIApplicationClearPrivateDataParams
(Assignee)

Updated

3 years ago
Blocks: 1168777
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
(Assignee)

Comment 9

3 years ago
update bug title per Jonas' comment in Comment 8
Summary: add cookieJar attribute in mozIApplicationClearPrivateDataParams → use cookieJar attribute to notify webapps-clear-cookiejar-data
(Assignee)

Updated

3 years ago
Summary: use cookieJar attribute to notify webapps-clear-cookiejar-data → Add cookieJar attribute and notify webapps-clear-cookiejar-data
(Assignee)

Comment 10

3 years ago
Created attachment 8612238 [details] [diff] [review]
WIP Patch

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

Comment 11

3 years ago
Created attachment 8612716 [details] [diff] [review]
WIP Patch v2.
Attachment #8612238 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8612791 [details] [diff] [review]
Patch

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
(Assignee)

Comment 13

3 years ago
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+
(Assignee)

Updated

3 years ago
Depends on: 1170097
(Assignee)

Comment 15

3 years ago
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.
(Assignee)

Comment 16

3 years ago
Created attachment 8613443 [details] [diff] [review]
Patch v2.

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)
(Assignee)

Comment 17

3 years ago
Created attachment 8613444 [details] [diff] [review]
interdiff (v1 - v2)
(Assignee)

Comment 18

3 years ago
Created attachment 8613447 [details] [diff] [review]
Patch v2.

qref
Attachment #8613443 - Attachment is obsolete: true
Attachment #8613443 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
Attachment #8613447 - Flags: review?(jonas)
(Assignee)

Comment 19

3 years ago
Created attachment 8613449 [details] [diff] [review]
interdiff (v1 - v2)
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.
(Assignee)

Updated

3 years ago
Attachment #8613449 - Flags: feedback?(bobbyholley)
(Assignee)

Comment 21

3 years ago
(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
(Assignee)

Updated

3 years ago
Attachment #8613447 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
Attachment #8613449 - Flags: feedback?(bobbyholley)
(Assignee)

Comment 22

3 years ago
Created attachment 8613463 [details] [diff] [review]
Patch. v3
Attachment #8613447 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8613463 - Flags: review?(jonas)
(Assignee)

Updated

3 years ago
Attachment #8613449 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Blocks: 1170115
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+
(Assignee)

Comment 24

3 years ago
Created attachment 8616544 [details] [diff] [review]
Patch. v4

addressed jonas' comments.
Attachment #8613463 - Attachment is obsolete: true
Attachment #8616544 - Flags: review+
(Assignee)

Updated

3 years ago
Summary: Add cookieJar attribute and notify webapps-clear-cookiejar-data → Add cookieJar attribute and notify clear-cookiejar-data
(Assignee)

Updated

3 years ago
Blocks: 1172815

Comment 25

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/55698478f5ba
(Assignee)

Comment 26

3 years ago
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
(Assignee)

Comment 27

3 years ago
(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)

Comment 29

3 years ago
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
(Assignee)

Updated

3 years ago
Blocks: 1175784
(Assignee)

Comment 31

3 years ago
Created attachment 8624009 [details] [diff] [review]
Patch v5.

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+

Comment 32

3 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/8ae5db65a77a
https://hg.mozilla.org/mozilla-central/rev/8ae5db65a77a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
No longer depends on: 1163254
Blocks: 1179985

Updated

2 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.