Closed Bug 1399590 Opened 3 years ago Closed 3 years ago

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)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: amchung)

Details

(Keywords: crash, intermittent-failure)

Crash Data

Attachments

(1 file, 4 obsolete files)

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]
Assignee: nobody → amchung
Priority: P5 → P3
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!
Attachment #8910171 - Flags: review?(josh)
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.
Attachment #8910171 - Flags: review?(josh) → review-
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!
Attachment #8910171 - Attachment is obsolete: true
Attachment #8912626 - Flags: review?(josh)
Sorry for uploading wrong file.
Attachment #8912626 - Attachment is obsolete: true
Attachment #8912626 - Flags: review?(josh)
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
Attachment #8912627 - Flags: review-
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!
Attachment #8912627 - Attachment is obsolete: true
Attachment #8913361 - Flags: review?(josh)
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"
Attachment #8913361 - Flags: review?(josh) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c4a8859b6c
Modify the argument of nsICookiePermission::CanAccess for changing nsIURI to nsIPrincipal. r=jdm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64c4a8859b6c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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?
Flags: needinfo?(amchung)
Hi Ryan,
This problem only occurs on test cases, in my view, this patch doesn't need to uplift to beta.
Flags: needinfo?(amchung)
You need to log in before you can comment on or make changes to this bug.