Last Comment Bug 246092 - nsContentPolicy caches Category list
: nsContentPolicy caches Category list
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Image Blocking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Christian Persch (GNOME) (away; not receiving bug mail)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 246085
Blocks:
  Show dependency treegraph
 
Reported: 2004-06-09 13:01 PDT by timeless
Modified: 2007-01-13 08:40 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
update mPolicies on category change (8.68 KB, patch)
2005-01-15 14:38 PST, Christian Persch (GNOME) (away; not receiving bug mail)
bzbarsky: superreview+
Details | Diff | Splinter Review
updated patch (8.91 KB, patch)
2005-01-18 11:47 PST, Christian Persch (GNOME) (away; not receiving bug mail)
no flags Details | Diff | Splinter Review
updated patch, taking into account the checked-in patch from bug 246085 (8.47 KB, patch)
2005-06-19 06:19 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
no flags Details | Diff | Splinter Review
updated patch (12.82 KB, patch)
2005-06-19 08:40 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
timwatt: review+
Details | Diff | Splinter Review
proxy category change notifications to main thread (2.83 KB, patch)
2005-07-11 11:25 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
no flags Details | Diff | Splinter Review
updated patch, adapted to notifications on main thread (9.34 KB, patch)
2005-07-17 06:41 PDT, Christian Persch (GNOME) (away; not receiving bug mail)
timwatt: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
new patch (8.69 KB, patch)
2006-03-05 04:46 PST, Christian Persch (GNOME) (away; not receiving bug mail)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description timeless 2004-06-09 13:01:37 PDT
Categories can change. nsContentPolicy should either listen to a hypothetical
obserer notification for category additions/removals (bug 246085), or get the
current category list before doing work.
Comment 1 Tim 2004-09-17 22:16:10 PDT
-> default owner, until I get some time to give this attention.
Comment 2 Christian Persch (GNOME) (away; not receiving bug mail) 2005-01-15 14:38:52 PST
Created attachment 171373 [details] [diff] [review]
update mPolicies on category change

This patch updates the list of content policies in mPolicies when the
content-policy category changes; needs the patch from bug 246085 (attachment
171372 [details] [diff] [review]) to work.
Comment 3 timeless 2005-01-16 14:23:54 PST
Comment on attachment 171373 [details] [diff] [review]
update mPolicies on category change

>Index: content/base/src/nsContentPolicy.cpp
>@@ -22,136 +22,169 @@
...
>+    UpdatePolicies();
>+
>+    nsresult rv;
>+    nsCOMPtr<nsIObserverService> observerService
>+        = do_GetService("@mozilla.org/observer-service;1", &rv);
>+    if (NS_FAILED(rv))
since you don't use rv past here, you can use the one arg form of do_GetService
and null check observerService instead of storing rv.
>+        return;
>+    observerService->AddObserver(this, "category-changed", PR_TRUE);
>+}

do you want to use the define from category manager?
>+    if (!strcmp (aTopic, "category-changed")) {
Comment 4 Tim 2005-01-16 15:05:17 PST
The code looks reasonable, given a couple assumptions (which I'll state, since
my memory of Mozilla threading is fuzzy).

Prerequisites for this patch being adequate:
1. All observer notifications occur in a serial fashion (we won't ever get
notified a second time while we are processing a notification).
2. All accesses to Content Policy are more or less serial (since mPolicies is in
an indeterminate state for the duration of UpdatePolicies, CheckPolicy must not
be run until UpdatePolicies finishes).

For example, if some somebody screws with the list while a web page is loading,
can we be sure that CheckPolicy will not overlap with UpdatePolicies?  If not,
this patch will not work (for that situation, anyway).

I'll hold my review until someone comments (in this bug or privately to me)
about the above.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-01-16 16:44:12 PST
Comment on attachment 171373 [details] [diff] [review]
update mPolicies on category change

>Index: content/base/src/nsContentPolicy.cpp
>+    observerService->AddObserver(this, "category-changed", 

Don't we need to RemoveObserver() even if we're being held as weak?  

>+nsContentPolicy::Observe(nsISupports     *aSubject,
>+    if (!strcmp (aTopic, "category-changed")) {


Kill space after "strcmp".

>+        nsAutoString categoryName (someData);

nsDependentString, please.

>Index: content/base/src/nsContentPolicy.h
>+class nsContentPolicy : public nsIContentPolicy,
>+			public nsIObserver,
>+			public nsSupportsWeakReference

Fix the indent here.

sr=bzbarsky with those nits fixed.
Comment 6 Christian Persch (GNOME) (away; not receiving bug mail) 2005-01-18 11:46:56 PST
(In reply to comment #5)
> (From update of attachment 171373 [details] [diff] [review] [edit])
> >Index: content/base/src/nsContentPolicy.cpp
> >+    observerService->AddObserver(this, "category-changed", 
> 
> Don't we need to RemoveObserver() even if we're being held as weak?  

Not sure where I'd place the RemoveObserver() call; if I do
do_GetService("@mozilla.org/observer-service;1"); in ~nsContentPolicy, it
returns null...

> >+nsContentPolicy::Observe(nsISupports     *aSubject,
> >+    if (!strcmp (aTopic, "category-changed")) {
> 
> Kill space after "strcmp".
Done.

> >+        nsAutoString categoryName (someData);
> 
> nsDependentString, please.
Fixed.
 
> >Index: content/base/src/nsContentPolicy.h
> >+class nsContentPolicy : public nsIContentPolicy,
> >+			public nsIObserver,
> >+			public nsSupportsWeakReference
> 
> Fix the indent here.
Fixed.

Attaching updated patch...

Comment 7 Christian Persch (GNOME) (away; not receiving bug mail) 2005-01-18 11:47:44 PST
Created attachment 171658 [details] [diff] [review]
updated patch
Comment 8 Darin Fisher 2005-01-18 12:49:48 PST
Calling RemoveObserver is not really necessary if this observer is meant to
exist for the lifetime of the app.  NS_ShutdownXPCOM causes the observer service
to remove any remaining observers after the xpcom-shutdown notification has been
dispatched.
Comment 9 Christian Persch (GNOME) (away; not receiving bug mail) 2005-06-18 06:55:33 PDT
(In reply to comment #4)
> The code looks reasonable, given a couple assumptions (which I'll state, since
> my memory of Mozilla threading is fuzzy).
> 
> Prerequisites for this patch being adequate:
> 1. All observer notifications occur in a serial fashion (we won't ever get
> notified a second time while we are processing a notification).
> 2. All accesses to Content Policy are more or less serial (since mPolicies is in
> an indeterminate state for the duration of UpdatePolicies, CheckPolicy must not
> be run until UpdatePolicies finishes).
> 
> For example, if some somebody screws with the list while a web page is loading,
> can we be sure that CheckPolicy will not overlap with UpdatePolicies?  If not,
> this patch will not work (for that situation, anyway).

The category could be changed from any thread (cat man is threadsafe). When the
category changes, the observer service's NotifyObservers is called on that
thread, and it notifies on the same thread. So, conceivably, there could be
another notification while one notification is being processed, on a different
thread, which would leave mPolicies in an undetermined state.

Solutions that I can think of are:
a) protect mPolicies with a lock, or
b) change the behaviour of the observer service wrt. threads: notifications
could be proxied to the thread the observer was added from (or maybe to the main
thread)
Comment 10 Christian Persch (GNOME) (away; not receiving bug mail) 2005-06-19 06:19:18 PDT
Created attachment 186726 [details] [diff] [review]
updated patch, taking into account the checked-in patch from bug 246085

And use a lock when accessing mPolicies.
Comment 11 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-19 06:50:26 PDT
Comment on attachment 186726 [details] [diff] [review]
updated patch, taking into account the checked-in patch from bug 246085

why not give the class an nsresult Init(); function which takes care of the
initialization?

Also, you could use nsAutoLock to ensure that all codepaths will unlock the
lock.
Comment 12 Christian Persch (GNOME) (away; not receiving bug mail) 2005-06-19 08:40:43 PDT
Created attachment 186733 [details] [diff] [review]
updated patch

Use nsAutoLock, and use a Init method for initialisation.
Comment 13 Tim 2005-06-19 22:48:35 PDT
Comment on attachment 186733 [details] [diff] [review]
updated patch

looks good, conditional on the following:

sr (or someone else) needs to verify that it compiles and runs

Keep an eye on Tp, since this will probably get called a bunch of times for
every page load. (Ts/Txul may also deserve watching if any of chrome gets
routed through content policy--I don't remember whether this is the case or
not.)

I'm going to dig up someone to verify it as mentioned before I + it.
Comment 14 Tim 2005-06-20 00:47:26 PDT
Comment on attachment 186733 [details] [diff] [review]
updated patch

thanks to WeirdAl for helping me verify it.
Comment 15 timeless 2005-06-21 12:14:12 PDT
Comment on attachment 186733 [details] [diff] [review]
updated patch

>Index: nsContentPolicy.cpp
>@@ -14,152 +14,221 @@
> nsContentPolicy::nsContentPolicy()
you should init mLock(nsnull) here, otherwise someone who fails to call Init
will crash.

>+nsContentPolicy::Init()
>+    if (observerService) {
tabs:
>+	if (NS_FAILED(rv))
>+            return rv;

>@@ -171,58 +240,60 @@ nsContentPolicy::CheckPolicy(CPMethod   
>+    nsAutoLock lock(mLock);
>      * Enumerate mPolicies and ask each of them, taking the logical AND of
>      * their permissions.
>     for (PRInt32 i = 0; i < count; i++) {
calling a method while locked isn't very nice and is probably asking for
deadlock.
>         rv = (policy->*policyMethod)(contentType, contentLocation,
>                                      requestingLocation, requestingContext,
>                                      mimeType, extra, decision);

>Index: nsContentPolicy.h
>@@ -15,63 +15,73 @@
Comment 16 Christian Persch (GNOME) (away; not receiving bug mail) 2005-06-27 12:03:59 PDT
(In reply to comment #15)
> (From update of attachment 186733 [details] [diff] [review] [edit])
> >Index: nsContentPolicy.cpp
> >@@ -14,152 +14,221 @@
> > nsContentPolicy::nsContentPolicy()
> you should init mLock(nsnull) here, otherwise someone who fails to call Init
> will crash.
> >+nsContentPolicy::Init()
> >+    if (observerService) {
> tabs:
> >+	if (NS_FAILED(rv))
> >+            return rv;
Both fixed in my tree.
 
> >@@ -171,58 +240,60 @@ nsContentPolicy::CheckPolicy(CPMethod   
> >+    nsAutoLock lock(mLock);
> >      * Enumerate mPolicies and ask each of them, taking the logical AND of
> >      * their permissions.
> >     for (PRInt32 i = 0; i < count; i++) {
> calling a method while locked isn't very nice and is probably asking for
> deadlock.
> >         rv = (policy->*policyMethod)(contentType, contentLocation,
> >                                      requestingLocation, requestingContext,
> >                                      mimeType, extra, decision);

I don't understand: under what circumstances would a evaluating a policy lead to
re-entering nsContentPolicy::ShouldLoad or ::ShouldProcess? As far as I can see,
that would mean that the policy tries to load something in the page; but why
should we allow that?
Comment 17 timeless 2005-06-27 13:23:14 PDT
i'm thinking more about a policy which decides during a shouldload call to
register for another xpcom category, which enters your observe method and gets
killed by nspr for locking twice.

there's a general rule: don't hold locks while calling outside your module. it
very much applies here.
Comment 18 Darin Fisher 2005-06-27 15:45:18 PDT
> there's a general rule: don't hold locks while calling outside your module. it
> very much applies here.

ditto!  even so called re-entrant locks (monitors) are not safe.  you should
push whatever state you need onto the stack, then exit the lock, and then
finally issue the observer callback based only on the state stored on the stack.

However, I now have a question:

+nsresult
+nsContentPolicy::Init()
+{
+    mLock = PR_NewLock();
+    if (!mLock) {
+        return NS_ERROR_OUT_OF_MEMORY;
+    }
+

Why does this object need a lock?

OK, I see the comment about the observer service's NotifyObservers method being
run on the thread that changed a category.  That is just majorly broken.  The
observer service is not designed with multiple threads in mind.  The category
service is also not well designed if it is triggering observer notifications on
a background thread.  I don't think this code (nsContentPolicy) should have to
be designed to be poked on background threads.  Gecko (in this case the stuff
under content/) is not threadsafe -- nor should it be -- and we definitely don't
want to incrementally make it so without a high level design.  Let's re-assess
why we are locking here, and see if we can't eliminate that.
Comment 19 Christian Persch (GNOME) (away; not receiving bug mail) 2005-07-06 14:18:48 PDT
> However, I now have a question:
> 
> +nsresult
> +nsContentPolicy::Init()
> +{
> +    mLock = PR_NewLock();
> +    if (!mLock) {
> +        return NS_ERROR_OUT_OF_MEMORY;
> +    }
> +
> 
> Why does this object need a lock?
> 
> OK, I see the comment about the observer service's NotifyObservers method being
> run on the thread that changed a category.  That is just majorly broken.  The
> observer service is not designed with multiple threads in mind.  The category
> service is also not well designed if it is triggering observer notifications on
> a background thread.  I don't think this code (nsContentPolicy) should have to
> be designed to be poked on background threads.  Gecko (in this case the stuff
> under content/) is not threadsafe -- nor should it be -- and we definitely don't
> want to incrementally make it so without a high level design.  Let's re-assess
> why we are locking here, and see if we can't eliminate that.

The observer service's NotifyObservers could be changed to call the observers'
::Observe on the main thread. Would that introduce too much of a performance
penalty for the most common case when NotifyObservers is already called on the
main thread?
Or alternatively and just for this bug, the category manager could make sure it
calls the observer service's NotifyObservers on the main thread.
Or the category manager could refuse category changes from other threads.

Which one would you prefer? Is there another way that I haven't found?
Comment 20 Darin Fisher 2005-07-06 17:51:19 PDT
I'm not 100% sure, but I think it probably makes sense to dispatch the observer
notifications to the main thread.  I would document this in the API someplace. 
Using XPCOM proxies, you can basically call a function on the main thread and
have it build and use a proxy only if called from a background thread.  That
way, there is no cost if calling the function on the main thread (which is the
common case).

  NS_GetProxyForObject(..., PROXY_ASYNC, ...);

Provided you do not OR in PROXY_ALWAYS, you will get the behavior I described.
Comment 21 timeless 2005-07-11 09:56:59 PDT
darin: speaking of broken our proxies don't support safely proxying a
threadunsafe object to the main thread such that it could participate in this
game, currently that the observer service pretends to be threadsafe at least
partially alleviates this mess.
Comment 22 Christian Persch (GNOME) (away; not receiving bug mail) 2005-07-11 11:25:22 PDT
Created attachment 188960 [details] [diff] [review]
proxy category change notifications to main thread

I'm not sure if in comment 20 darin meant that all notifications ought to be
sent to main thread, or just the category change notifications. Here's a patch
that implements the latter.
Comment 23 Darin Fisher 2005-07-11 12:46:45 PDT
It might make sense to proxy all observer service notifications to the main
thread, but that seems like a more involved change.  At this point, we probably
want to minimize the scope of the changes, so the suggested patch seems best to me.
Comment 24 Christian Persch (GNOME) (away; not receiving bug mail) 2005-07-17 06:41:12 PDT
Created attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

I posted the patch for proxying the notifications to the main thread in bug
246085. Unless I'm misunderstanding how threads work, that should make it
impossible to get to GetPolicies while a ShouldLoad or ShouldProcess is being
processed, so no locks are needed anymore. That should address Tim's concerns
in comment 4.
Comment 25 Tim 2005-07-17 12:13:31 PDT
Comment on attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

For future reference, it's handy when you make patches from mozilla/, but that
isn't really important.

It looks good, but hold it until bug 246085 gets that patch checked in (unless
someone thinks the other one will make it before the next alpha/beta/release of
the various tier-1 products).

It doesn't seem to break anything in seamonkey or firefox (today's CVS) by my
90-second "load-some-pages" testing. (Note I only tested it with the 246085
patch, but my understanding is that the problem this solves doesn't exist in
main-line anyway--that it only affects third-party extensions/embedders).

If anyone has a useful test case (extension, etc.) that verifies that this
really fixes the problem, speak up.
Comment 26 Christian Persch (GNOME) (away; not receiving bug mail) 2005-12-05 04:54:24 PST
Comment on attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

(In reply to comment #25)
> (From update of attachment 189594 [details] [diff] [review] [edit])
> It looks good, but hold it until bug 246085 gets that patch checked in 
That patch has been checked in.

Asking for sr now.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2005-12-05 11:42:17 PST
Comment on attachment 189594 [details] [diff] [review]
updated patch, adapted to notifications on main thread

>Index: nsContentPolicy.cpp
> nsContentPolicy::nsContentPolicy()
> {
> #ifdef PR_LOGGING
>     if (! gConPolLog) {
>         gConPolLog = PR_NewLogModule("nsContentPolicy");

That can fail, no?  This should probably live in Init() as well.

sr=bzbarsky with that addressed.
Comment 28 Benjamin Smedberg [:bsmedberg] 2005-12-06 10:24:44 PST
we use static PRLogModule *gSuchandSuchLog = PR_NewLogModule(); all over the code, I think we should treat it as an acceptable pattern
Comment 29 Christian Persch (GNOME) (away; not receiving bug mail) 2006-03-05 04:46:51 PST
Created attachment 214081 [details] [diff] [review]
new patch

I've gone back and ripped out most of my new code, since I can now simply use a nsCategoryCache<nsIContentPolicy> which keeps the list in sync with the category for me.

I didn't move the PR_NewLogModule to Init since I removed Init again; also if this is really a problem it should be done in a new bug (since this pattern is commonly used in many places).
Comment 30 Christian :Biesinger (don't email me, ping me on IRC) 2006-03-05 05:18:35 PST
Comment on attachment 214081 [details] [diff] [review]
new patch

I wonder if nsCategoryCache should have a Count() and operator[] function...
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2006-03-05 10:27:26 PST
Comment on attachment 214081 [details] [diff] [review]
new patch

r+sr=bzbarsky
Comment 32 timeless 2006-03-21 03:02:18 PST
Comment on attachment 214081 [details] [diff] [review]
new patch

mozilla/content/base/src/nsContentPolicy.h 	3.10
mozilla/content/base/src/nsContentPolicy.cpp 	3.16
mozilla/content/base/src/nsContentPolicy.cpp 	3.17
Comment 33 Phil Ringnalda (:philor) 2006-12-29 20:49:35 PST
So, is this fixed, and just not marked fixed?

Note You need to log in before you can comment on or make changes to this bug.