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

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

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).

Comment 1

[Kris Maglione [:kmag]]

(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.

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

(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 3

[Kris Maglione [:kmag]]

Attachment: Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter.

Review commit: https://reviewboard.mozilla.org/r/253020/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253020/

Comment 4

[Shane Caraveo (:mixedpuppy)]

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

https://reviewboard.mozilla.org/r/253020/#review259650

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d28d3a80e781ae10cbc97775415827ec93193c56
Bug 1470965: Fix refcount sanity in nsIPrincipal.addonPolicy getter. r=mixedpuppy

Comment 6

[Stefan Hindli [:stefan_hindli]]

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

Comment 7

[Kris Maglione [:kmag]]

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

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/25c2eeca526e

Comment 9

[Kris Maglione [:kmag]]

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.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/adfbbc7a8f60

Comment 12

[Victor Carciu]

Marking as qe-verify- based on https://bugzilla.mozilla.org/show_bug.cgi?id=1470965#c9