Allow more than one global redirect observer

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
Networking
P2
enhancement
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

({fixed1.8.1})

Trunk
mozilla1.8.1
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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
Created attachment 211646 [details] [diff] [review]
patch

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 2

13 years ago
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-
Created attachment 211977 [details] [diff] [review]
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.
Attachment #211646 - Attachment is obsolete: true
Attachment #211977 - Flags: review?(darin)
Created attachment 211978 [details]
interdiff between v1 and v2

Comment 5

13 years ago
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+
Created attachment 212244 [details] [diff] [review]
patch v2.1

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
Created attachment 212638 [details] [diff] [review]
patch for checkin
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
Created attachment 212741 [details] [diff] [review]
patch for MOZILLA_1_8_BRANCH

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)

Updated

12 years ago
Attachment #212741 - Flags: approval-branch-1.8.1?(darin) → approval-branch-1.8.1+
fixed on branch
Keywords: fixed1.8.1

Comment 17

12 years ago
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 19

12 years ago
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...

Comment 21

12 years ago
> 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.