1280775 - Crash in nsPermissionManager::RemovePermission when removing an already-removed website’s offline data

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Emmanuel Gil Peyrot]

This bug was filed from the Socorro interface and is 
report bp-706fdaf9-ae97-4f77-b676-067b22160618.
=============================================================

Steps to reproduce:
- Go to about:preferences > Advanced > Network
- Select a website in “Offline Web Content and User Data”
- Select “Remove…” and press on it twice
- Accept the removal in the first dialog
- On accepting the removal in the second dialog, Firefox will crash.

Comment 1

[Alice0775 White]

The problem is reproduce since Firefox42, not on Firefox41.

Seems regressed by Bug 1173523

Comment 2

[Alice0775 White]

and Bug 1201365

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Bug 1280775 - null check in removePermission to prevent crash: permission manager change.

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

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Bug 1280775 - null check in removePermission to prevent crash: add test.

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

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8763632 [details]
Bug 1280775 - null check in removePermission to prevent crash: permission manager change.

https://reviewboard.mozilla.org/r/59808/#review56900

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8763633 [details]
Bug 1280775 - null check in removePermission to prevent crash: add test.

https://reviewboard.mozilla.org/r/59810/#review56904

lgtm

Comment 7

[Nika Layzell [:nika] (ni? for response)]

https://reviewboard.mozilla.org/r/59808/#review56902

::: extensions/cookie/nsPermissionManager.cpp:1871
(Diff revision 1)
>  NS_IMETHODIMP
>  nsPermissionManager::RemovePermission(nsIPermission* aPerm)
>  {
> +  if (!aPerm) {
> +    return NS_OK;
> +  }

I don't have a problem with this check, but I think that we should also perform a null check in advanced.js => removeOfflineApp to check that getPermissionObject actually returns a non-null object before trying to call matchesURI or remove the permission.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8763632 [details]
Bug 1280775 - null check in removePermission to prevent crash: permission manager change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59808/diff/1-2/

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8763633 [details]
Bug 1280775 - null check in removePermission to prevent crash: add test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59810/diff/1-2/

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to Michael Layzell [:mystor] from comment #7)
> I don't have a problem with this check, but I think that we should also
> perform a null check in advanced.js => removeOfflineApp to check that
> getPermissionObject actually returns a non-null object before trying to call
> matchesURI or remove the permission.
Ok, put that for loop and the removePermission call into an |if (perm)|. I didn't exit the method early so zombie permissions get still removed from the UI at its bottom. The STR with doubleclick don't work for me, so I test like this:
1. Open http://html5demos.com/offlineapp
2. Open Options/Preferences and go to Advanced > Network.
3. Copy the url of the Options tab.
4. Open a second browser window.
5. Paste and load the url from the clipboard.
6. Go to Advanced > Network.
7. Remove permission in one window.
8. Remove permission in other window.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Since this is not a new regression, we'll try to fix it in 48+.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

What's the current status of this bug? It is probably too late to take a fix for 48 but maybe we can make 49 (aurora)...

Comment 13

[Nika Layzell [:nika] (ni? for response)]

(In reply to David Bolter [:davidb] from comment #12)
> What's the current status of this bug? It is probably too late to take a fix
> for 48 but maybe we can make 49 (aurora)...

I think this patch could land right now.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acc54d397653
null check in removePermission to prevent crash: permission manager change. r=mystor
https://hg.mozilla.org/integration/autoland/rev/489b889ae02f
null check in removePermission to prevent crash: add test. r=mystor

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/acc54d397653
https://hg.mozilla.org/mozilla-central/rev/489b889ae02f

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8763632 [details]
Bug 1280775 - null check in removePermission to prevent crash: permission manager change.

Sorry for the unresponsiveness.

Approval Request Comment (also for the attachment with the test)
[Feature/regressing bug #]: bug 1173523
[User impact if declined]: Firefox will crash if requests twice to remove offline data but accepts the confirmation dialogs only after clicking the button twice (edge case)
[Describe test coverage new/current, TreeHerder]: Tested locally, landed on integration and central https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=489b889ae02f182fdf3dd12c0b9e3c470732f674&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable Also added a test.
[Risks and why]: low (added if conditions and a test)
[String/UUID change made/needed]: none

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8763632 [details]
Bug 1280775 - null check in removePermission to prevent crash: permission manager change.

Crash fix, Aurora50+

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2c68973fc56
https://hg.mozilla.org/releases/mozilla-aurora/rev/bdd8924b8ebb