Comment 79 for bug 294712

(In reply to comment #74)
> >+ OverridePermissionEntry() {};
>
> >+static nsTArray<OverridePermissionEntry> mOverridePermissionEntries;
>
> This will show up as a leak. Why exactly do you need this as a static global
> instead of a member on nsContentBlocker?

I thought that createInstance on nsIContentBlocker might make two instances of this array, but that's probably wrong anyhow. I'll fix that.

> >+nsContentBlocker::IgnorePolicy(nsIURI *aURI,
> >+ NS_ENSURE_ARG(aContentType);
>
> I'm not sure why you're doing that, since you're not actually making sure it's
> a "valid" content type.

Well, XPConnect throws an error when an invalid _type_ is used, but it won't complain if |null| is used. But I should probably just change this to |if (!aContentType || aContentType < ...)...|, is that fine?

> >+ nsCString host;
> >+ nsresult rv = aURI->GetHost(host);
> >+ NS_ENSURE_SUCCESS(rv, rv);
>
> Did you actually test this? It looks like with your patch calling reloadImage
> on a data: image, say, will throw an exception on the ignorePolicy() call and
> that you don't bother catching that. I don't know whether you want to change
> this code or the caller, but one or the other needs changing. I should note
> that your IDL doesn't talk about the fact that this method will throw on all
> sorts of URIs...

Yeah, I need to change this. I think this shouldn't throw, just return NS_OK. On the caller side, I'll try to grab the principal uri of the image so that data images embedded in websites will work. Does that work? Will the imgIRequest uri for that image reflect the principal uri on a data image?

> >+ const int loc = FindPermission(aURI, aContentType);
> >+ if (loc != -1)
> >+ mOverridePermissionEntries.RemoveElementAt(loc);
>
> This looks bad to me. In particular if I ignorePolicy, then someone else does
> ignorePolicy, then they restorePolicy they'll restore both ignores. If that's
> not desirable, then we should either throw on nested ignore or properly have an
> ignore/restore stack or something.

Ok, I'll do the ignore/restore stack. I'll also document in the idl that the way to delete all overrides for any given content-type + url is by checking isPolicyOverriden and repeatedly restoring the policy. Something like |while(isPolicyOverriden(url, con))restorePolicy(url, con);|

> I'm not sure why ignorePolicy and restorePolicy are returning "the current
> policy", given the above.

The return values are what I felt was most appropriate, if you want them to just be void return values I can do that as well. ignorePolicy changes a policy so it returns that policy. restorePolicy brings back a policy so it returns that. The caller is presumably aware of what he is overriding/restoring so it makes sense to return a value that he might not be aware of.

> Also, shouldn't this compare to nsIContentPolicy::ACCEPT, since that's what the
> callers will be passing in?

The way TestPermission works is it uses the local defines in the file, which are just redefinitions of the nsIContentPolicy constants. So, BEHAVIOR_ACCEPT is always nsIContentPolicy::ACCEPT

> It looks like overriding with REJECT_* is just ignored. Is that actually on
> purpose? If so, the API should reflect that, I would think.

They're not ignored at all, in fact each particular REJECT_* value will have a different effect just as if you would make a real policy change. TestPermission has a very strange way of implementing the 3 policies, and I say 3 because that's how it was setup. It uses the aFromPref out value to indicate whether the rejection is REJECT_TYPE or REJECT_SERVER. So, aPermission will indicate whether or not it is rejected and aFromPref indicates what type of rejection it was. A third-party rejection is rejected the same way as REJECT_SERVER, from TestPermission's pov.

> >+ * @param aContentLocation The location for which to override content policies.
> >+ * The policy is ignored for the domain of the site.
>
> This should document what happens if the URI passed in doesn't have a
> meaningful "domain" (whatever that is; are these policies really per-host and
> not at least per hostport?)

They're per host (by which I probably mean hostport, not sure what GetHost returns, I'll check and document).

> >+ * @returns The current nsContentBlocker policy for this content/domain.
>
> Here "current" means "after this call completes", right?

Yes.