1191653 - Listen to clear-origin-data in nsPermissionManager.cpp

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

We should listen to clear-origin-data instead of webapps-clear-data,

See comments from Jonas in https://bugzilla.mozilla.org/show_bug.cgi?id=1168300#c8

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

this will also change nsIPermissionManager.removePermissionsForApp, so making this blocks Bug 1179985

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: WIP Patch

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6e9dfa83b2f

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8673535 [details] [diff] [review]
Patch.

Review of attachment 8673535 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/ChromeUtils.cpp
@@ +30,5 @@
> +
> +/* static */ bool
> +ChromeUtils::OriginAttributesMatchPattern(const dom::OriginAttributesDictionary& aAttrs,
> +                                          const dom::OriginAttributesPatternDictionary& aPattern)
> +{

Please revert the changes in ChromeUtils, and just use |pattern.Matches| directly in any C++ consumers - there's no reason that code needs to go through ChromeUtils.

::: extensions/cookie/nsPermissionManager.cpp
@@ +7,5 @@
>  #include "mozilla/DebugOnly.h"
>  
>  #include "mozilla/dom/ContentParent.h"
>  #include "mozilla/dom/ContentChild.h"
> +#include "mozilla/dom/ChromeUtils.h"

Please revert this.

@@ +48,5 @@
>  static nsPermissionManager *gPermissionManager = nullptr;
>  
>  using mozilla::dom::ContentParent;
>  using mozilla::dom::ContentChild;
> +using mozilla::dom::ChromeUtils;

And this.

@@ +2321,5 @@
>      if (NS_FAILED(rv)) {
>        continue;
>      }
>  
> +    if (!ChromeUtils::OriginAttributesMatchPattern(mozilla::BasePrincipal::Cast(principal)->OriginAttributesRef(), aPattern)) {

As noted above, please just do:

!aPattern.Matches(mozilla::BasePrincipal::Cast(principal)->OriginAttributesRef());

::: extensions/cookie/nsPermissionManager.h
@@ +205,5 @@
>     */
> +  static void ClearOriginDataObserverInit();
> +
> +  nsresult
> +  RemovePermissionsForOriginAttributes(mozilla::OriginAttributesPattern& aAttrs);

Please call this RemovePermissionsWithAttributes

::: extensions/cookie/test/unit/test_permmanager_cleardata.js
@@ +10,4 @@
>  }
>  
> +// Return the data required by 'clear-origin-data' notification.
> +function getData(aOriginAttributes)

This should be "aPattern", right?

::: netwerk/base/nsIPermissionManager.idl
@@ +238,2 @@
>     */
> +  void removePermissionsForOriginAttributes(in DOMString pattern);

You should specify here that this is JSON. I'd call it "patternAsJSON" or "jsonPattern" or something.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch. v2

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/48a895231734

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/48a895231734

Comment 9

[Ethan Tseng [:ethan]]

Nice job! Thanks Yoshi!