Closed Bug 777072 Opened 8 years ago Closed 8 years ago

Update Permission Manager database to handle apps and mozbrowser

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
blocking-basecamp +

People

(Reporter: mounir, Assigned: mounir)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(8 files, 3 obsolete files)

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
Permissions should not be given per-origin but per { origin, app, mozbrowser }.
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
(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: ? → +
I think next steps are updating nsPermission, IPC::Permission and finally the database.
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+
With leak/test failures being fixed.
Attachment #652765 - Attachment is obsolete: true
Attachment #653870 - Flags: review?(jonas)
Now, I have to write tests and fix a leak I haven't been able to reproduce locally yet.
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.
Blocks: 619236
(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 ;)
Attached patch Part 7 - TestsSplinter Review
Attachment #654064 - Flags: review?(jonas)
Depends on: 785161
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 → ---
Blocks: 785631
Can't tell if something is testable from an end-user perspective or not.
Whiteboard: [qa?]
QA Contact: jsmith
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-]
Depends on: 787717
Depends on: 814554
You need to log in before you can comment on or make changes to this bug.