WebExtensionPolicy.cpp raises "Assertion failure: int32_t(mRefCnt) > 0 (dup release)" after accessing addonPolicy property on a principal object instance

RESOLVED FIXED in Firefox 62

Status

RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: rpl, Assigned: kmag)

Tracking

(Depends on: 1 bug, {regression})

unspecified
mozilla63
regression
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox62 fixed, firefox63 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 months ago
This bug is a follow up for Bug 1459613 (See Bug 1459613 comment 10), to investigate a bit deeper on an assertion failure raised (by WebExtensionPolicy.cpp) that has been caught in Bug 1459613 by some test-verify-e10s failures on the windows platform, e.g.:

https://treeherder.mozilla.org/logviewer.html#?job_id=184172724&repo=try&lineNumber=1259)

The assertion failures seems to be related to the part of Bug 1459613's patch applied on PermissionUI.jsm, in particular the principalName getter that retrieve  the name to be used in the permission doorhanger.

The version that fails on TV was retrieving the extension name by accessing `this.principal.addonPolicy.name`, and the TV failure has been fixed by retrieving the `WebExtensionPolicy` instance from the principal URI using WebExtensionPolicy.getByURI, and so it seems that the assertion fails because the principal is keeping a reference to it (and the principal is probably also kept active by the PermissionUI.jsm internals).
(Assignee)

Updated

9 months ago
Assignee: nobody → kmaglione+bmo
(Assignee)

Comment 1

9 months ago
(In reply to Luca Greco [:rpl] from comment #0)
> The version that fails on TV was retrieving the extension name by accessing
> `this.principal.addonPolicy.name`, and the TV failure has been fixed by
> retrieving the `WebExtensionPolicy` instance from the principal URI using
> WebExtensionPolicy.getByURI, and so it seems that the assertion fails
> because the principal is keeping a reference to it (and the principal is
> probably also kept active by the PermissionUI.jsm internals).

That is not a fix.
(Reporter)

Comment 2

9 months ago
(In reply to Kris Maglione [:kmag] from comment #1)
> (In reply to Luca Greco [:rpl] from comment #0)
> > The version that fails on TV was retrieving the extension name by accessing
> > `this.principal.addonPolicy.name`, and the TV failure has been fixed by
> > retrieving the `WebExtensionPolicy` instance from the principal URI using
> > WebExtensionPolicy.getByURI, and so it seems that the assertion fails
> > because the principal is keeping a reference to it (and the principal is
> > probably also kept active by the PermissionUI.jsm internals).
> 
> That is not a fix.

yeah, I know, that is a workaround for the underlying issue and in fact I wrote: 

"the TV failure has been fixed..."

The reason why I tried to apply the workaround and push the patch again on try is that 
I wanted to confirm which part of the patch was actually triggering it before filing an
issue for the failed assertion, and I mentioned the workaround in case it was useful
while looking into the real fix (and I wasn't suggesting to use the workaround as a fix,
obviously).
Comment hidden (mozreview-request)

Comment 4

9 months ago
mozreview-review
Comment on attachment 8987746 [details]
Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter.

https://reviewboard.mozilla.org/r/253020/#review259650
Attachment #8987746 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 5

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28d3a80e781ae10cbc97775415827ec93193c56
Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter. r=mixedpuppy
Backed out 3 changesets (bug 1470023, bug 1469719, bug 1470965) for | toolkit/components/perfmonitoring/tests/browser/browser_compartments.js on a CLOSED TREE 

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=bab121b4dd84f9715e6a9efa652556a91ea60a3c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=43040128202efc47d4249e623e6d3ffd1a5d9588&filter-classifiedState=unclassified&selectedJob=185004097

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=24aeaadfae0df355fb2fac7ea25ea90fa9c56ddf&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Log: https://treeherder.mozilla.org/logviewer.html#?job_id=185004097&repo=mozilla-inbound&lineNumber=2513

12:29:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Error: Error: No matching message handler :: Async*background@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:3:11
12:29:59     INFO - async*@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:1:17
12:29:59     INFO -  - 
12:29:59     INFO - Stack trace:
12:29:59     INFO -     background@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:187:7
12:29:59     INFO -     async*@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:1:17
12:29:59     INFO -     
12:29:59     INFO - Not taking screenshot here: see the one that was previously logged
12:29:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | executeScript - 
12:29:59     INFO - Stack trace:
12:29:59     INFO -     background@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:188:7
12:29:59     INFO -     async*@moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/%7B62a81496-888d-f242-8ef8-442b57bff1c8%7D.js:1:17
12:29:59     INFO -     
12:29:59     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | test result correct - 
12:29:59     INFO - Console message: [JavaScript Error: "Promise rejected after context unloaded: Message manager disconnected
12:29:59     INFO - " {file: "moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/script.js" line: 2}]
12:29:59     INFO - @moz-extension://3ee86764-b762-5340-a5bf-c15c8f281ced/script.js:2:9
12:29:59     INFO - 
12:29:59     INFO - Console message: Ignoring response to aborted listener for 1099511627780
12:29:59     INFO - Console message: Cannot send function call result: other side closed connection (call data: ({path:"tabs.executeScript", args:[409, {allFrames:null, code:"42", cssOrigin:null, file:null, frameId:null, matchAboutBlank:null, runAt:"document_idle"}]}))
12:29:59     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Message manager count - 
12:29:59     INFO - TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Pending response count - 
12:29:59     INFO - Leaving test bound testExecuteScript
12:29:59     INFO - Console message: Ignoring response to aborted listener for 1668
12:29:59     INFO - Console message: Cannot send function call result: other side closed connection (call data: ({path:"tabs.executeScript", args:[410, {allFrames:null, code:"location.href", cssOrigin:null, file:null, frameId:null, matchAboutBlank:null, runAt:"document_idle"}]}))
12:29:59     INFO - GECKO(869) | MEMORY STAT | vsize 4724MB | residentFast 794MB | heapAllocated 190MB
12:29:59     INFO - TEST-OK | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | took 939ms
12:29:59     INFO - Not taking screenshot here: see the one that was previously logged
12:29:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Found an unexpected tab at the end of test run: about:blank - 
12:29:59     INFO - Not taking screenshot here: see the one that was previously logged
12:29:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Found an unexpected tab at the end of test run: http://example.com/ - 
12:29:59     INFO - Not taking screenshot here: see the one that was previously logged
12:29:59     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_tabs_executeScript.js | Found an unexpected tab at the end of test run: http://example.net/ - 
12:29:59     INFO - checking window state
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 7

9 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/25c2eeca526e1248bdb3d5f88a5bfce86ab1d8c7
Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter. r=mixedpuppy

Comment 8

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25c2eeca526e
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Updated

9 months ago
Blocks: 1459613
Depends on: 1345346
(Assignee)

Updated

7 months ago
Blocks: 1448604
(Assignee)

Comment 9

7 months ago
Comment on attachment 8987746 [details]
Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1396449
[User impact if declined]: This issue causes random crashes in certain places when a principal's addonPolicy property is accessed, due to reference counting errors.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: I verified manually.
[Needs manual test from QE? If yes, steps to reproduce]: N/A
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a trivial and obvious fix for a reference counting error.
[String changes made/needed]: None.
Flags: needinfo?(kmaglione+bmo)
Attachment #8987746 - Flags: approval-mozilla-beta?
status-firefox62: --- → affected
Comment on attachment 8987746 [details]
Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter.

Crash fix, baked in nightly, let's take this for beta 17.
Attachment #8987746 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 11

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/adfbbc7a8f60
status-firefox62: affected → fixed

Comment 12

7 months ago
Marking as qe-verify- based on https://bugzilla.mozilla.org/show_bug.cgi?id=1470965#c9
Flags: qe-verify-
Keywords: regression
You need to log in before you can comment on or make changes to this bug.