315598 - Allow more than one global redirect observer

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

from bug 248052

it'd be nice if extensions could register to receive all redirect notifications, either by using a category to create nsIChannelEventSink impls, or by using nsIObserverService to broadcast notifications.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

most of this patch is nsCategoryCache, which is useful for at least two other bugs, and the testcase.

Comment 2

[Darin Fisher]

Comment on attachment 211646 [details] [diff] [review]
patch

>Index: base/public/Makefile.in

>+		nsCategoryCache.h \

This feels like something that may be of more general use.  Perhaps
it should be added to xpcom/components?


>Index: base/src/nsBaseChannel.cpp

>-  // Give the global event sink a chance to observe/block this redirect.
>-  nsCOMPtr<nsIChannelEventSink> channelEventSink =
>-      do_GetService(NS_GLOBAL_CHANNELEVENTSINK_CONTRACTID);
>-  if (channelEventSink) {
>-    rv = channelEventSink->OnChannelRedirect(this, newChannel, redirectFlags);
>-    if (NS_FAILED(rv))
>-      return rv;
>-  }
...
>   // Give our consumer a chance to observe/block this redirect.
>   GetCallback(channelEventSink);
>   if (channelEventSink) {
>     rv = channelEventSink->OnChannelRedirect(this, newChannel, redirectFlags);
>     if (NS_FAILED(rv))
>       return rv;
>   }
> 
>+  // Global observers
>+  NS_ASSERTION(gIOService, "Must have an IO service");
>+  rv = gIOService->OnChannelRedirect(this, newChannel, redirectFlags);
>+  if (NS_FAILED(rv))
>+    return rv;

Originally, I wanted to give the script security manager first dibs on
trumping a redirect.  That way, other consumers would not see redirects
that are not going to happen because of security checks.  That still
seems like a good idea.


>Index: base/src/nsCategoryCache.cpp

>+nsCategoryObserver::nsCategoryObserver(const char* aCategory,
>+                                       nsCategoryListener* aListener)
>+  : mListener(aListener), mCategory(aCategory)
>+{
>+  if (!mHash.Init()) {
>+    // OOM
>+    mListener = nsnull;
>+    return;
>+  }

You may as well wait to initialize mListener until after this check
to reduce codesize a bit.


>Index: base/src/nsIOService.cpp

> NS_IMPL_THREADSAFE_ISUPPORTS4(nsIOService,
>                               nsIIOService,
>+                              nsIIOService,
>                               nsINetUtil,
>                               nsIObserver,
>                               nsISupportsWeakReference)

Is this a typo?


>+    PRInt32 len = entries.Count();
>+    for (int i = 0; i < len; ++i) {

s/int/PRInt32/ for type consistency


>Index: build/nsNetCID.h

>+#define NS_REDIRECT_OBSERVER_CATEGORY "redirect-observers"

Perhaps NS_CHANNEL_EVENT_SINK_CATEGORY would be a better name.  Also,
how about "net-channel-event-sinks" or something like that?


>Index: test/unit/test_event_sink.js

>+const NS_BINDING_ABORTED = 0x804b0002;

We really need to add these to Components.results ;-)

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

>You may as well wait to initialize mListener until after this check
>to reduce codesize a bit.

did that, but not sure whether it reduces codesize, as I have to init it to null at least in the failure case.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: interdiff between v1 and v2

Comment 5

[Darin Fisher]

Comment on attachment 211977 [details] [diff] [review]
patch v2

looks good.  you might need to add something to dlldeps.cpp to avoid link errors on windows.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2.1

now with dlldeps.cpp

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

oh, hm, I'll need to add a #include there too. please pretend that it's there ;)

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 212244 [details] [diff] [review]
patch v2.1

>+++ b/xpcom/components/nsCategoryCache.h
>+    // no virtual destructor (people shouldn't delete through an
>+    // nsCategoryNotification pointer)

What's nsCategoryNotification?

>+ * This is a helper class that caches services that are registered in a certain
>+ * category. The intended usage is that a service stores a variable of type
>+ * nsCategoryCache<nsIFoo> in a member variable, where nsIFoo is the interface
>+ * that these services should implement. The constructor of this class should
>+ * then get the name of the category.

Is there any way for the consumer to "clean up" (say at shutdown)?  As things stand, if you hold a ref to yourself through an nsCategoryCache you can't help leaking, can you?

With those nits, sr=bzbarsky

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #8)
> (From update of attachment 212244 [details] [diff] [review] [edit])
> >+++ b/xpcom/components/nsCategoryCache.h
> >+    // no virtual destructor (people shouldn't delete through an
> >+    // nsCategoryNotification pointer)
> 
> What's nsCategoryNotification?

Oh... the former name of nsCategoryListener. I'll fix the comment.

> Is there any way for the consumer to "clean up" (say at shutdown)?  As things
> stand, if you hold a ref to yourself through an nsCategoryCache you can't help
> leaking, can you?

Hm? How do you manage to hold a ref to yourself that way?

Comment 10

[Boris Zbarsky [:bzbarsky]]

You hold a ref to some other service that holds a ref to you, say.  That's the simplest non-contrived scenario.

As in, I want this stuff to listen for shutdown in a useful way and drop all refs when it happens, basically.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

oh yeah... I didn't consider that part when writing this

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch for checkin

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

(this observes xpcom-shutdown now and releases all references then)

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on trunk.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch for MOZILLA_1_8_BRANCH

Branch version of the patch. This includes adding the http server for the unit tests on the branch.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed on branch

Comment 17

[Jon Henry]

Could this checkin be responsible for the regression reported in bug 328225? The Forecastfox extension is suddenly causing Firefox to become extremely unstable, and this bug is about the only major change in the regression window.

Comment 18

[François Gagné]

This patch seems to be the cause of the new #1 crasher on MOZILLA_1_8_BRANCH (Firefox 2.0). See http://talkback-public.mozilla.org/reports/firefox/FF2x/FF2x-topcrashers.html.

Some talkbacks lead to http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/protocol/http/src/nsHttpHandler.cpp&mark=494&rev=MOZILLA_1_8_BRANCH#494

Comment 19

[Darin Fisher]

So, perhaps gIOService is null in some cases where the HTTP channel is still executing.  Hmm...

Comment 20

[Boris Zbarsky [:bzbarsky]]

Most of those crashes are at 

nsIOService::OnChannelRedirect  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/netwerk/base/src/nsIOService.cpp, line 262]

Which on the 1.8 branch is:

262                            PRInt32 len = entries.Count();

which is really odd...

Comment 21

[Darin Fisher]

> 262                            PRInt32 len = entries.Count();
> 
> which is really odd...

Here's my take on it:

nsIOService::OnChannelRedirect is non-virtual, so it is possible to call OnChannelRedirect on a null nsIOService pointer.  It doesn't crash until it tries to dereference |this|, which happens one line above line 262.  Combined with the fact that talkback is usually off by one line in crash reports, I'd say that everything points to calling OnChannelRedirect on a bogus (probably null) gIOService pointer.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  In that case, I bet bug 328218 accounts for the majority of the crashes we're seeing here.