1399590 - Intermittent PROCESS-CRASH | Main app process exited normally | application crashed [@ nsPermissionManager::CommonTestPermissionInternal(nsIPrincipal *,nsIURI *,char const *,unsigned int *,bool,bool)] after Assertion: PermissionAvaliable(prin, aType)

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P3)

Description

[Treeherder Bug Filer]

Filed by: hskupin [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=130580915&repo=try

https://queue.taskcluster.net/v1/task/AMDmsE6gRoSq7qdZGMw7fA/runs/0/artifacts/public/logs/live_backing.log

Here the first 10 frames from the crashing thread:

08:34:09     INFO -  GECKO(3648) | Assertion failure: PermissionAvaliable(prin, aType), at z:/build/build/src/extensions/cookie/nsPermissionManager.cpp:2265
08:34:23     INFO -  GECKO(3648) | #01: nsPermissionManager::CommonTestPermissionInternal(nsIPrincipal *,nsIURI *,char const *,unsigned int *,bool,bool) [extensions/cookie/nsPermissionManager.cpp:2221]
08:34:23     INFO -  GECKO(3648) | #02: nsPermissionManager::TestPermission(nsIURI *,char const *,unsigned int *) [extensions/cookie/nsPermissionManager.cpp:2118]
08:34:23     INFO -  GECKO(3648) | #03: nsCookiePermission::CanAccess(nsIURI *,nsIChannel *,int *) [extensions/cookie/nsCookiePermission.cpp:162]
08:34:23     INFO -  GECKO(3648) | #04: nsCookieService::CheckPrefs(nsICookiePermission *,unsigned char,bool,nsIURI *,bool,char const *,int) [netwerk/cookie/nsCookieService.cpp:4198]
08:34:23     INFO -  GECKO(3648) | #05: mozilla::net::CookieServiceChild::SetCookieStringInternal(nsIURI *,nsIChannel *,char const *,char const *,bool) [netwerk/cookie/CookieServiceChild.cpp:523]
08:34:23     INFO -  GECKO(3648) | #06: mozilla::net::CookieServiceChild::SetCookieString(nsIURI *,nsIPrompt *,char const *,nsIChannel *) [netwerk/cookie/CookieServiceChild.cpp:597]
08:34:23     INFO -  GECKO(3648) | #07: nsHTMLDocument::SetCookie(nsTSubstring<char16_t> const &,mozilla::ErrorResult &) [dom/html/nsHTMLDocument.cpp:1425]
08:34:23     INFO -  GECKO(3648) | #08: mozilla::dom::HTMLDocumentBinding::set_cookie [s3:gecko-generated-sources:dabf923a208dba88f3a912d4b46c841a83a82fe2d13420b769c5f00b2de55afb547ec47df1d88138ddc40e0bf81b51150cc80f9ff28433daad20a340bd5e61b1/dom/bindings/HTMLDocumentBinding.cpp::123]
08:34:23     INFO -  GECKO(3648) | #09: mozilla::dom::GenericBindingSetter(JSContext *,unsigned int,JS::Value *) [dom/bindings/BindingUtils.cpp:3011]
08:34:23     INFO -  GECKO(3648) | #10: js::CallJSNative(JSContext *,bool (*)(JSContext *,unsigned int,JS::Value *),JS::CallArgs const &) [js/src/jscntxtinlines.h:293]

Comment 1

[Intermittent Failures Robot]

2 failures in 1032 pushes (0.002 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 2

Platform breakdown:
* windows7-32: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1399590&startday=2017-09-11&endday=2017-09-17&tree=all

Comment 2

[Amy Chung [:Amy]]

Attachment: implementation -- Added a flag of confirming mozbrowser and if mozbrowser is true, we don't have to do async cookie.

Hi Josh,
I found if mozbrowser is true in non-e10s mode, the permission key of child is different from parent on oop tests.
In my view, we have to verify dom.mozBrowserFramesEnabled and network.cookie.ipc.sync before doing async cookie.

Would you help me to review this patch?
Thanks!

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 8910171 [details] [diff] [review]
implementation -- Added a flag of confirming mozbrowser and if mozbrowser is true, we don't have to do async cookie.

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

Now that we have time to solve this issue the right way, we should do so. I've talked with mystor, and the problem is that we're only providing a URI to nsPermissionManager::TestPermission. If we used nsPermissionManager::TestPermissionFromPrincipal, this would not be an issue. Therefore, I propose:
* we modify nsICookiePermission::CanAccess (and nsCookiePermission) to accept an nsIPrincipal argument instead of nsIURI (and remove the unused nsIChannel argument)
* we modify nsCookieService::CheckPrefs to accept an nsIPrincipal argument instead of nsIURI
All of the callers of these methods appear to have either a principal already available, or OriginAttributes available that can be used to create a codebase principal from the existing URI.

With this change, we should be able to remove the use of sync IPC from browserElement_CookiesNotThirdParty.js.

Comment 4

[Amy Chung [:Amy]]

Attachment: implementation -- Modified the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal.

Hi Josh,
I have modified this patch as below:
[nsICookiePermission]
1. Modified CanAccess(nsIURI, nsIChannel) to CanAccess(nsIPrincipal).
2. Modified nsCookiePermission::CanAccess() for changing mPermMgr->TestPermission() to mPermMgr->TestPermissionFromPrincipal().

[CookieService]
1. Added argument of OriginAttribbutes on CheckPref().

[Test Case]
1. Removed the use of sync IPC from browserElement_CookiesNotThirdParty.js.

Would you help me to review this patch?
Thanks!

Comment 5

[Amy Chung [:Amy]]

Attachment: implementation -- Modified the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal.

Sorry for uploading wrong file.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 8912627 [details] [diff] [review]
implementation -- Modified the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal.

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

The changes generally look good, but let's ensure that we don't change behaviour unintentionally.

::: dom/base/Navigator.cpp
@@ -605,5 @@
>  
> -  nsCOMPtr<nsIURI> codebaseURI;
> -  doc->NodePrincipal()->GetURI(getter_AddRefs(codebaseURI));
> -
> -  if (!codebaseURI) {

Let's keep this code as it is so we don't change behaviour here.

::: extensions/cookie/nsCookiePermission.cpp
@@ -145,5 @@
>  {
> -  // Check this protocol doesn't allow cookies
> -  bool hasFlags;
> -  nsresult rv =
> -    NS_URIChainHasFlags(aURI, nsIProtocolHandler::URI_FORBIDS_COOKIE_ACCESS,

We need to keep this code, so we should extract a URI from the principal in order to check it.

::: netwerk/cookie/nsCookieService.cpp
@@ +4190,5 @@
> +  nsCOMPtr<nsIPrincipal> principal =
> +      BasePrincipal::CreateCodebasePrincipal(aHostURI, aOriginAttrs);
> +
> +  if (!principal) {
> +    return STATUS_REJECTED_WITH_ERROR;

Add a COOKIE_LOGFAILURE here so we don't get a silent failure.

::: netwerk/cookie/nsICookiePermission.idl
@@ +55,5 @@
>     * this method is called to test whether or not the given URI/channel may
>     * access the cookie database, either to set or get cookies.
>     *
> +   * @param aPrincipal
> +   *        the principal for confirming perssion.

the principal trying to access cookies

Comment 7

[Amy Chung [:Amy]]

Attachment: implementation -- Modified the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal.

Hi Josh,
I have modified the patch from your suggestions as below:
1. Reverted the modification on Navigator.cpp.
2. Reverted the modification of confirming hasFlags on nsCookiePermission.cpp.
3. Added COOKIE_LOGFAILURE when principal can't create on nsCookieService.cpp.
4. Modified the comment on nsICookiePermission.idl.

Would you help me to review this patch?
Thanks!

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8913361 [details] [diff] [review]
implementation -- Modified the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal.

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

::: netwerk/cookie/nsCookieService.cpp
@@ +4190,5 @@
> +  nsCOMPtr<nsIPrincipal> principal =
> +      BasePrincipal::CreateCodebasePrincipal(aHostURI, aOriginAttrs);
> +
> +  if (!principal) {
> +    COOKIE_LOGFAILURE(aCookieHeader ? SET_COOKIE : GET_COOKIE, aHostURI, aCookieHeader, "can't creat principal.");

"non-codebase principals cannot get/set cookies"

Comment 9

[Amy Chung [:Amy]]

Attachment: implementation -- Modified the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal.

[Try Server]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bfb1a943990da2c9894fa5e1493f747019f7a95

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c4a8859b6c
Modify the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal. r=jdm

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/64c4a8859b6c

Comment 12

[Ryan VanderMeulen [:RyanVM]]

I'm not seeing any instances of this on OrangeFactor, but I believe the code in question also exists on Beta? Is there a user impact that justifies backport here or can this ride the 58 train?

Comment 13

[Amy Chung [:Amy]]

Hi Ryan,
This problem only occurs on test cases, in my view, this patch doesn't need to uplift to beta.