Closed Bug 315598 Opened 14 years ago Closed 14 years ago

Allow more than one global redirect observer

Categories

(Core :: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.8.1

People

(Reporter: Biesinger, Assigned: Biesinger)

References

Details

(Keywords: fixed1.8.1)

Attachments

(2 files, 4 obsolete files)

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.
Severity: normal → enhancement
OS: Linux → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
most of this patch is nsCategoryCache, which is useful for at least two other bugs, and the testcase.
Attachment #211646 - Flags: review?(darin)
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla1.8.1
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 ;-)
Attachment #211646 - Flags: review?(darin) → review-
Attached patch patch v2 (obsolete) — Splinter Review
>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.
Attachment #211646 - Attachment is obsolete: true
Attachment #211977 - Flags: review?(darin)
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.
Attachment #211977 - Flags: review?(darin) → review+
Attached patch patch v2.1 (obsolete) — Splinter Review
now with dlldeps.cpp
Attachment #211977 - Attachment is obsolete: true
Attachment #212244 - Flags: superreview?(bzbarsky)
oh, hm, I'll need to add a #include there too. please pretend that it's there ;)
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
Attachment #212244 - Flags: superreview?(bzbarsky) → superreview+
(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?
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.
oh yeah... I didn't consider that part when writing this
Attachment #211978 - Attachment is obsolete: true
Attachment #212244 - Attachment is obsolete: true
(this observes xpcom-shutdown now and releases all references then)
fixed on trunk.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Branch version of the patch. This includes adding the http server for the unit tests on the branch.
Attachment #212741 - Flags: approval-branch-1.8.1?(darin)
Attachment #212741 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
fixed on branch
Keywords: fixed1.8.1
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.
So, perhaps gIOService is null in some cases where the HTTP channel is still executing.  Hmm...
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...
> 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.
Ah, ok.  In that case, I bet bug 328218 accounts for the majority of the crashes we're seeing here.
You need to log in before you can comment on or make changes to this bug.