Closed Bug 1211591 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Permission Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: bkelly, Assigned: nika)

References

Details

Attachments

(2 files, 1 obsolete file)

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)
Attachment #8669853 - Flags: review?(bkelly)
Attachment #8669853 - Flags: review?(bkelly) → review?(josh)
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 :)
Attachment #8669853 - Flags: review?(josh) → review+
Re-uploaded with correct r=jdm - because why not
Attachment #8669853 - Attachment is obsolete: true
Here are some tests for RemovePermissionForApp.
Attachment #8670321 - Flags: review?(josh)
Attachment #8670321 - Flags: review?(josh) → review+
You need to log in before you can comment on or make changes to this bug.