1211591 - nsPermissionsManager.cpp references non-existent appId column in moz_perms table

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

See this:

  http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsPermissionManager.cpp#2327

Which was introduced in this changeset:

  http://hg.mozilla.org/mozilla-central/rev/d4cc20fc3973

Comment 1

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

Attachment: Remove incorrect sql statement from nsPermissionManager::RemovePermissionsForApp

This initial query to clear the database was initially removed because of the removal of the appId column, and replaced with the following logic which parses the origin strings to obtain the appId property, and then removes those entries using `AddInternal`. Unfortunately, during a rebase, it appears as though the code crept back into the codebase.

I'm not sure why this codepath wasn't ever tested before bkelly noticed it, but we should probably add a test of some form which actually runs the RemovePermissionsForApp codepath, to ensure that it actually works.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d063a0c5c2c9

(I think that this patch should work correctly, and fix the problem, but I would also want to add another test, to make sure that this code path isn't neglected in the future)

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 8669853 [details] [diff] [review]
Remove incorrect sql statement from nsPermissionManager::RemovePermissionsForApp

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

Please ensure this is tested before merging :)

Comment 3

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

Attachment: Part 1: Remove incorrect sql statement from nsPermissionManager::RemovePermissionsForApp

Re-uploaded with correct r=jdm - because why not

Comment 4

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

Attachment: Part 2: Tests for nsPermissionManager::RemovePermissionsForApp

Here are some tests for RemovePermissionForApp.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b75225d75f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/becb0ef36136

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/0b75225d75f7
https://hg.mozilla.org/mozilla-central/rev/becb0ef36136