Update Permission Manager database to handle apps and mozbrowser

RESOLVED FIXED in mozilla17

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks 1 bug)

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(8 attachments, 3 obsolete attachments)

9.93 KB, patch
sicking
: review+
Details | Diff | Splinter Review
16.83 KB, patch
sicking
: review+
Details | Diff | Splinter Review
10.24 KB, patch
sicking
: review+
Details | Diff | Splinter Review
7.64 KB, patch
sicking
: review+
Details | Diff | Splinter Review
19.66 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.57 KB, patch
sicking
: review+
Details | Diff | Splinter Review
5.89 KB, patch
sicking
: review+
Details | Diff | Splinter Review
30.53 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
Assignee

Description

7 years ago
Permissions should not be given per-origin but per { origin, app, mozbrowser }.
Assignee

Comment 1

7 years ago
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #645495 - Flags: review?(jonas)
Comment on attachment 645496 [details] [diff] [review]
Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal

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

I'll try to fix this up.

::: extensions/cookie/nsPermissionManager.cpp
@@ +304,5 @@
>        const IPC::Permission &perm = perms[i];
> +
> +      nsCOMPtr<nsIPrincipal> principal;
> +      nsresult rv = GetPrincipalForHost(perm.host, getter_AddRefs(principal));
> +      NS_ENSURE_SUCCESS(rv, rv);

This doesn't seem right. We should receive data from the parent process which includes appId+isInBrowserElement
Assignee

Comment 4

7 years ago
(In reply to Jonas Sicking (:sicking) from comment #3)
> Comment on attachment 645496 [details] [diff] [review]
> Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal
> 
> Review of attachment 645496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll try to fix this up.
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +304,5 @@
> >        const IPC::Permission &perm = perms[i];
> > +
> > +      nsCOMPtr<nsIPrincipal> principal;
> > +      nsresult rv = GetPrincipalForHost(perm.host, getter_AddRefs(principal));
> > +      NS_ENSURE_SUCCESS(rv, rv);
> 
> This doesn't seem right. We should receive data from the parent process
> which includes appId+isInBrowserElement

The idea was to fix that in another part actually.
blocking-basecamp: ? → +
Assignee

Comment 6

7 years ago
I think next steps are updating nsPermission, IPC::Permission and finally the database.
Assignee

Comment 7

7 years ago
I introduced syntax errors with last minute changes.
Attachment #652761 - Attachment is obsolete: true
Attachment #652761 - Flags: review?(jonas)
Attachment #652765 - Flags: review?(jonas)
Comment on attachment 652765 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model

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

::: extensions/cookie/nsPermissionManager.h
@@ +53,5 @@
> +  /**
> +   * PermissionKey is the key used by PermissionHashKey hash table.
> +   *
> +   * NOTE: It could be implementing nsIHashable but there is no reason to worry
> +   * with XPCOM interfaces while we don't need to.

"when we don't need to."
Attachment #652765 - Flags: review?(jonas) → review+
Assignee

Comment 9

7 years ago
With leak/test failures being fixed.
Attachment #652765 - Attachment is obsolete: true
Assignee

Comment 12

7 years ago
Attachment #653870 - Flags: review?(jonas)
Assignee

Comment 13

7 years ago
Now, I have to write tests and fix a leak I haven't been able to reproduce locally yet.
Assignee

Comment 14

7 years ago
I will merge this with Part 6 before pushing.
Attachment #653942 - Flags: review?(jonas)
Hmm... do you have code to deal with upgrading existing databases in the user's profile? We don't want to lose any user data when the user installs the next version of firefox.
Assignee

Updated

7 years ago
Blocks: 619236
Assignee

Comment 16

7 years ago
(In reply to Jonas Sicking (:sicking) from comment #15)
> Hmm... do you have code to deal with upgrading existing databases in the
> user's profile? We don't want to lose any user data when the user installs
> the next version of firefox.

This is done in "Part 6". You reviewed that patch 2 or 3 hours ago ;)
Assignee

Comment 17

7 years ago
Attachment #654064 - Flags: review?(jonas)
Assignee

Updated

7 years ago
Depends on: 785161
Assignee

Comment 18

7 years ago
Justin, could you review the tests changes. Everything else has been reviewed by Jonas.
Attachment #653810 - Attachment is obsolete: true
Attachment #654716 - Flags: review?(justin.lebar+bug)
Comment on attachment 654716 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking)

Yeesh.
Attachment #654716 - Flags: review?(justin.lebar+bug) → review+
Backed out, sorry

https://hg.mozilla.org/mozilla-central/rev/eccef9b3f1f1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Updated

7 years ago
Blocks: 785631
Can't tell if something is testable from an end-user perspective or not.
Whiteboard: [qa?]
QA Contact: jsmith
Assignee

Comment 24

7 years ago
It is testable from an end-user perspective but we have automated tests for that so I think it's not worth doing manual testings.
(In reply to Mounir Lamouri (:mounir) from comment #24)
> It is testable from an end-user perspective but we have automated tests for
> that so I think it's not worth doing manual testings.

Okay sounds good.
Whiteboard: [qa?] → [qa-]
Assignee

Updated

7 years ago
Depends on: 787717

Updated

7 years ago
Depends on: 814554
You need to log in before you can comment on or make changes to this bug.