191380 - Rewrite permission code (for cookies, images etc)

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Michiel van Leeuwen (email: mvl+moz@)]

The current code for managing permission for cookies, images and popups is all
in the cookies code. And it is not very clean.
It should be removed form extensions/cookie and made be more generic.

Also, a more clean UI is needed.

This is related to bug 187304.

This bug is for tracking that.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

-> me
Adding people to CC

Comment 2

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: phase 1, version 1

Attaching the first version to clean up the permission code.

It's functions:

Move stuff from nsPermissions.cpp to nsPermissionManager.cpp
Create seperate handlers for each consumer (cookies, images, popup)
Clean up some code
Prepare for moving out of extensions/cookie (so that cookies can go into necko)

No new functionality, but prepare for whitelisting

Also see http://yellow.exedo.nl/~michiel/permissions.html for what the next
steps should be.

Comment 3

[Darin Fisher]

Comment on attachment 116200 [details] [diff] [review]
phase 1, version 1

i might not get to this until next week.  sorry!

Comment 4

[[e-mail]]

Attachment: review comments

review comments - mostly nits, a couple more cleanup requests, and that's about
it. looking good! can't wait for this to get in the tree :)

hopefully darin will get to look at this tomorrow or early next week, so we can
get it in before the freeze.

there are some specific questions I have for darin regarding parts of the
patch, I'll post them separately for convenience.

Comment 5

[[e-mail]]

Attachment: review questions for darin

uhh, sorry for all the questions ;)

they're all taken from the main review attachment, so if you lose context, you
can find them in there.

Comment 6

[[e-mail]]

freeze? what freeze?

get more sleep, dwitte.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: phase1, version 2

Changed according to most of dwitte's comments, plus as bonus making profile
switching work again. dwitte, thanks for the review!

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: review answers

Answers to comments I didn't address in the patch, and why.
Things I left out here are fixed.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: mac stuff

Mac build stuff.
Is this still needed? Anyway, as a seperate patch, because i forgot it before.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: phase 1, version 3

I introduced a crash, when opening the media tab in page properties. Attaching
updated patch. Sorry for all the spam.

Changes since previous patch:
--- extensions/cookie/nsImgManager.cpp	14 Mar 2003 18:59:52 -0000
+++ extensions/cookie/nsImgManager.cpp	14 Mar 2003 22:03:14 -0000
@@ -148,10 +148,10 @@
     NS_ASSERTION(content, "no content available");
     if (content) {
       rv = content->GetDocument(*getter_AddRefs(doc));
-      if (NS_FAILED(rv)) return rv;
+      if (NS_FAILED(rv) || !doc) return rv;
 
       rv = doc->GetBaseURL(*getter_AddRefs(baseURI));
-      if (NS_FAILED(rv)) return rv;
+      if (NS_FAILED(rv) || !baseURI) return rv;

Comment 11

[Darin Fisher]

instead of this:

static const char kHeader[] = "# HTTP Permission File\n#
http://www.netscape.com/newsref/std/cookie_spec.html\n# This is a generated
file!  Do not edit.\n\n";

try this:

static const char kHeader[] =
    "# HTTP Permission File\n"                            
    "# http://www.netscape.com/newsref/std/cookie_spec.html\n"
    "# This is a generated file!  Do not edit.\n\n";

it just reads better.

i haven't given this a completely thorough review yet.  overall, it looks solid.
 i'll try to finish my review shortly.

Comment 12

[Darin Fisher]

Attachment: response to "review questions for darin"

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Phase 1, version 4

Updated patch, now includes mac build changes.
Addresses more of dwitte's comments (the nsVoidArray comments, the order of
arguments to Add(), some comments), and things darin mentioned.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: interdiff between v3 and v4

for reviewers, changes between v3 and v4

Comment 15

[[e-mail]]

Comment on attachment 117318 [details] [diff] [review]
Phase 1, version 4

r=dwitte

looks good!

thx for the response to those review questions, darin. also, that multiple-line
kHeader thing is neat, I didn't know you could do that.

btw, about the 'deleting permissionfile on shutdown-cleanse' thing, I didn't
catch your answer... are you unsure, or did you want us to change those?

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

I forgot to mention that this patch removes the need for nsImages.cpp and
nsPermissions.cpp. They can be removed.

Comment 17

[[e-mail]]

oh, one more note. Once this patch and my cookie patch (bug 195908) land, we'll
have removed all dependencies on nsUtils.h and nsUtils.cpp - we can cvs remove them.

can we get r/sr for the makefile changes and cvs remove of those two files in
advance?

(so when both patches land I can whip up a makefile patch, and get whoever lands
them to take care of it at the same time)

Comment 18

[Darin Fisher]

> btw, about the 'deleting permissionfile on shutdown-cleanse' thing, I didn't
> catch your answer... are you unsure, or did you want us to change those?

i would address this particular problem in a different bug.  i'm not exactly
sure what we want to do, and others might have some good input.  it's probably
not an urgent issue, so i'm fine if you guys want to punt on it for now.

Comment 19

[[e-mail]]

>i would address this particular problem in a different bug.  i'm not exactly
>sure what we want to do, and others might have some good input.  it's probably
>not an urgent issue, so i'm fine if you guys want to punt on it for now.

heh, i was thinking the same thing. i'll go file a bug against conrad to check
(though i suspect his answer will be "leave it the way it is")

Comment 20

[Darin Fisher]

Comment on attachment 117318 [details] [diff] [review]
Phase 1, version 4

>Index: extensions/cookie/nsCookiePermission.cpp

>+const PRUint32 kDefaultPolicy = nsIPermissionManager::ALLOW_ACTION;

declare this static.

>+nsCookiePermission::Init()
>+{
>+  nsresult rv;
>+  // Continue on an error. We can do a few things without a permission manager
>+  mPermissionManager = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);
>+  return NS_OK;
>+}

then no need to capture |rv|.  use simpler do_GetService(ContractID) form.


>Index: extensions/cookie/nsCookiePromptService.cpp
>Index: extensions/cookie/nsCookies.cpp
>Index: extensions/cookie/nsICookiePermission.idl
>Index: extensions/cookie/nsICookiePromptService.idl
>Index: extensions/cookie/nsIImgManager.idl
>Index: extensions/cookie/nsIPermission.idl
>Index: extensions/cookie/nsIPermissionManager.idl
>Index: extensions/cookie/nsImgManager.cpp

>+  // On error, just don't use the host based lookup anymore. We can do the
>+  // other things, like mailnews blocking
>+  mPermissionManager = do_GetService(NS_PERMISSIONMANAGER_CONTRACTID, &rv);

another case where you don't need to capture |rv|.


>+  if (NS_FAILED(rv)) {
>+    NS_WARNING("Error occured reading image preferences");
>+  }

NS_ASSERTION(NS_SUCCEEDED(rv), "...");

although the optimizer will probably have no trouble eliminating this
branch in optimized code.  i'm undecided... take your pick.


>+    // Search for two dots, starting at the end.
>+    // If there are no two dots found, ++dot will turn to zero,
>+    // that will return the entire string.
>+    PRInt32 dot = currentHost.RFindChar('.');
>+    dot = currentHost.RFindChar('.', dot-1);
>+    ++dot;

hmm... ok, you're depending on RFindChar to always return -1 on failure and to
accept -2 as an offset.  possibly a bit fragile, but seems to be ok.  anything
else would require more code... alright.


>+  // check the topic, and the cached prefservice
>+  if (!mPrefBranch) {
>+    return NS_ERROR_FAILURE;
>+  }

under what conditions is this the case?  do we want to spit out a warning?
NS_ENSURE_TRUE?  or should this indeed be silent?


>+    nsCAutoString pref = NS_ConvertUCS2toUTF8(aData);

      NS_ConvertUCS2toUTF8 pref(aData); // NS_ConvertUCS2toUTF8 is-a
nsCAutoString


>+  // check the prefservice is cached
>+  if (!mPrefBranch) {
>+    return NS_ERROR_FAILURE;
>+  }

same 'warning' question here?


>Index: extensions/cookie/nsPermissionManager.cpp

>+    nsVoidArray *mPermissionList;
>+};

>+  mPermissionList = new nsVoidArray();

why do you need to heap allocate the nsVoidArray?  it is a very
lightweight container by itself.  seems like you could save yourself
some trouble and just allocate it inline as a member var.


>+  nsCAutoString host(aHost);
...
>+  if (NS_SUCCEEDED(rv)) {
>+    uri->GetHostPort(host);
>+    if (host.IsEmpty())
>+      return NS_ERROR_FAILURE;
>+  }

better to only copy |aHost| into |host| if NS_NewURI fails, right?


>+  if (!permissionEnum) {
>+    *aEnum = nsnull;
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

FWIW, you are not required to assign out params on failure.  however,
a lot of code doesn't check return values, so take care if you wish
to eliminate |*aEnum = nsnull|.


>+    for (PRInt32 typeIndex = typeCount-1; typeIndex >= 0; --typeIndex) {
>+      hostStruct->permissionList.RemoveElementAt(typeIndex);
>+    }
>+    // no more types are present, remove the entry
>+    mPermissionList->RemoveElementAt(hostIndex);
>+    delete hostStruct;

i must have overlooked this part when reading the patch, but if you
are simply removing elements from the list, where do you delete those
elements?  aren't you leaking memory here?


>Index: extensions/cookie/nsPermissionManager.h

>+  nsVoidArray                 *mPermissionList;

another candidate for not being dynamically allocated.


>Index: extensions/cookie/nsPopupWindowManager.cpp

>+  nsCAutoString pref = NS_ConvertUCS2toUTF8(aData);

    NS_ConvertUCS2toUTF8 pref(aData);


address these issues and sr=darin.  sorry it took me so long to review your
patch!
i'm not going to be available until sunday night or monday, so unless you need
to
discuss something with me... i think you're good to go.

Comment 21

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Phase 1, version 5

Updated to darins and dwittes comments. Thanks for the reviews!

Comment 22

[[e-mail]]

Comment on attachment 118073 [details] [diff] [review]
Phase 1, version 5

nice changes. asked for a few more optimizations as well.

r=dwitte

Comment 23

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: interdiff between v4 and v5

Diff between v4 and v5. Interdiff was chocking, so this might not apply, but is
shows the changes.

Comment 24

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Phase 1, version 6

Fixed a problem where the tools->cookie manager items showed up as always
disabled.
And made it apply again.

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 118112 [details] [diff] [review]
Phase 1, version 6

Carrying over r=dwitte and sr=darin (per comment #20)

Comment 26

[[e-mail]]

checked in, thx bz!

since bug 195908 and this one are fixed, we can cvs remove nsImages.{cpp,h},
nsPermissions.{cpp,h}, and nsUtils.{cpp,h}. waiting for a few build cycles
before we go ahead and do this...

Comment 27

[[e-mail]]

Attachment: patch to remove nsUtils from makefiles

removes nsUtils from the makefiles; so it's ready for cvs removal

Comment 28

[Boris Zbarsky [:bzbarsky]]

Attachment: patch to remove dead code

Comment 29

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Attempt to fix Camino bustage

nsIPermission changed from char ** to nsACstring, and from PRBool to PRUint32
for permission. I missed camino. Hope this will fix it.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 118131 [details] [diff] [review]
Attempt to fix Camino bustage

>       if ( [[aTableColumn identifier] isEqualToString:@"Website"] ) {
>         // website url column
>-        nsXPIDLCString host;
>-        perm->GetHost(getter_Copies(host));
>+        nsCAutoString host;
>+        perm->GetHost(host);
>         retVal = [NSString stringWithCString:host];

I just checked this patch in, although I suspect that might need to be
|host.get()| on the last line there rather than just |host|.  We'll see what
tinderbox says.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 118131 [details] [diff] [review]
Attempt to fix Camino bustage

I changed it to host.get() after looking at some other files.

Comment 32

[[e-mail]]

closing this one out (see bug 195908 comment 37). great work mvl!

Comment 33

[[e-mail]]

Comment on attachment 118112 [details] [diff] [review]
Phase 1, version 6

>           // but cancel if it's an unsuitable URL
>           const PM = Components.classes["@mozilla.org/PopupWindowManager;1"]
>                      .getService(CI.nsIPopupWindowManager);
>-          if (!PM.testSuitability(this.popupURL))
>-            this.popupURL = null;
>         }
Um, yes, really useful change here... plus you forgot to update all of the rest of the callers in this file. I guess nobody uses this, since there doesn't seem to be a bug filed on the regression...