Closed Bug 804655 Opened 12 years ago Closed 12 years ago

HTTP channels should update the transaction and connection's callbacks when updating the channel loadgroup or callbacks

Categories

(Core :: Networking: HTTP, defect)

Product:
Core
Component:
Networking: HTTP
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19

People

(Reporter: jdm, Assigned: jdm)

References

Details

Attachments

(3 files, 8 obsolete files)

[FOR REFERENCE] Merge of attachemnt 677542 and attachemnt 679396 by Josh Matthews
37.12 KB, patch
mayhemer
: review-
Details | Diff | Splinter Review
Part 1: Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.
15.48 KB, patch
Details | Diff | Splinter Review
Part 2: Wrap up interface aggregator callbacks with a target thread on which they should be released.
44.83 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review 
      No description provided.
Comment on attachment 674268 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Review of attachment 674268 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. 

I suspect Patrick will want to know about this code, and may also be able to get to the review quicker?

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +200,5 @@
> +    {     return fwdObject ? (fwdObject)->BytesWritten() : 0; } \
> +    void UpdateCallbacks(nsIInterfaceRequestor* aCallbacks)    \
> +    {                                                          \
> +        return (fwdObject)->UpdateCallbacks(aCallbacks);        \
> +    }

nit: keep '\' in same column except for lines that are longer (i.e. your first line)
Attachment #674268 - Flags: review?(mcmanus)
Attachment #674268 - Flags: review?(cbiesinger)
Attachment #674268 - Flags: feedback+
Comment on attachment 674268 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Review of attachment 674268 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsAHttpConnection.h
@@ +199,5 @@
>      int64_t BytesWritten()                                  \
> +    {     return fwdObject ? (fwdObject)->BytesWritten() : 0; } \
> +    void UpdateCallbacks(nsIInterfaceRequestor* aCallbacks)    \
> +    {                                                          \
> +        return (fwdObject)->UpdateCallbacks(aCallbacks);        \

void fx shouldn't return a value

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5923,5 @@
>      gHttpHandler->OnExamineCachedResponse(this);
>  
>  }
>  
> +NS_IMETHODIMP

don't nsHttpBaseChannel::SetLoadGroup/SetNotificationCallbacks need to be marked virtual?

@@ +5925,5 @@
>  }
>  
> +NS_IMETHODIMP
> +nsHttpChannel::SetLoadGroup(nsILoadGroup *aLoadGroup)
> +{

can you assert both of these methods are called on the main thread?

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +391,5 @@
>  void
>  nsHttpTransaction::GetSecurityCallbacks(nsIInterfaceRequestor **cb,
>                                          nsIEventTarget        **target)
>  {
> +    mozilla::MutexAutoLock lock(mCallbacksLock);

this file is already using namespace mozilla (though it has a couple legacy mozilla::'s in it)

@@ +1521,5 @@
>  
> +void
> +nsHttpTransaction::UpdateCallbacks(nsIInterfaceRequestor* aCallbacks)
> +{
> +    mozilla::MutexAutoLock lock(mCallbacksLock);

redundant namespace
Attachment #674268 - Flags: review?(mcmanus) → review+
>don't nsHttpBaseChannel::SetLoadGroup/SetNotificationCallbacks need to be marked virtual?

Nope. Part of NS_IMETHOD is the virtual tag.
BTW, are you sure usage of the lock is correct?  I would like to check this patch as well, but I won't get to it today, only tomorrow.
The lock protects the only uses of mCallbacks - the getter is copying the pointers and addrefing them, so I don't see any ways it could cause a problem.
Comment on attachment 674268 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Review of attachment 674268 [details] [diff] [review]:
-----------------------------------------------------------------

First, regardless this patch, this whole callback thing is a big hack, I'm not the only one saying this.

With your patch you allow change to callbacks down to security objects of NSS to "something" since the underlying connection can be shared by many windows-ed and windows-less channels.

Few comments before you land:

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5945,5 @@
> +        nsCOMPtr<nsIInterfaceRequestor> callbacks;
> +        NS_NewNotificationCallbacksAggregation(mCallbacks, mLoadGroup,
> +                                               getter_AddRefs(callbacks));
> +        mTransaction->UpdateCallbacks(callbacks);
> +    }

This code could be shared.  We do this also when we setup a transaction on other places.  Having a single internal method would be nice.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +115,5 @@
>      // nsIResumableChannel
>      NS_IMETHOD ResumeAt(uint64_t startPos, const nsACString& entityID);
>  
> +    NS_IMETHODIMP SetNotificationCallbacks(nsIInterfaceRequestor *aCallbacks);
> +    NS_IMETHODIMP SetLoadGroup(nsILoadGroup *aLoadGroup);

Shouldn't this be NS_IMETHOD ?

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1523,5 @@
> +nsHttpTransaction::UpdateCallbacks(nsIInterfaceRequestor* aCallbacks)
> +{
> +    mozilla::MutexAutoLock lock(mCallbacksLock);
> +    mCallbacks = aCallbacks;
> +    mConnection->UpdateCallbacks(aCallbacks);

Please don't call foreign code while holding this class lock.  It may lead to unexpected behavior.  Protect just assignment of mCallbacks.


With this line I somehow sense a problem.  Connection's callbacks can be queried on any thread potentially.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +87,3 @@
>      nsIEventTarget        *ConsumerTarget() { return mConsumerTarget; }
>  
> +    void UpdateCallbacks(nsIInterfaceRequestor* aCallbacks);

Nit: maybe call this SetSecurityCallbacks for consistency with the rest of the while http code?
I added a mutex to nsHttpConnection. While it looks like there _exists_ the potential for deadlock (with regards to doing something with the callbacks, then causing a sync runnable to fire that calls GetInterface on the main thread while the mutex is still locked on the other thread), I don't believe this is actually a concern due to the code we execute inside the mutex on other threads.
Attachment #676611 - Flags: review?(honzab.moz)
Attachment #674268 - Attachment is obsolete: true
Whoops, forgot to update SetSecurityCallbacks to use the mutex.
Attachment #676614 - Flags: review?(honzab.moz)
Attachment #676611 - Attachment is obsolete: true
Attachment #676611 - Flags: review?(honzab.moz)
(In reply to Josh Matthews [:jdm] from comment #11)
> https://tbpl.mozilla.org/?tree=Try&rev=c81cc39d6d35

Kinda crashy... the parent is green, btw: https://tbpl.mozilla.org/?rev=972986c4f75e

I think just running xpcshell tests for network would tell you there is an issue before pushing to try ;)
Comment on attachment 676614 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Review of attachment 676614 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping r based on failing try.
Attachment #676614 - Flags: review?(honzab.moz)
https://tbpl.mozilla.org/?tree=Try&rev=bec794baac5en. Green this time. I promise that I did run the browser and browse around before asking for review on the previous patch :)
Attachment #676614 - Attachment is obsolete: true
Comment on attachment 676825 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Review of attachment 676825 [details] [diff] [review]:
-----------------------------------------------------------------

I have to admit that I'm a bit scared of this patch and the new API, but we will see what all it will bring to us...

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1578,2 @@
>      if (mCallbacks)
>          return mCallbacks->GetInterface(iid, result);

Please rather copy callbacks to a local variable under the lock and then GI out of the lock.

@@ +1583,5 @@
> +void
> +nsHttpConnection::SetSecurityCallbacks(nsIInterfaceRequestor* aCallbacks)
> +{
> +    MutexAutoLock lock(mCallbacksLock);
> +    mCallbacks = aCallbacks;

And what about the target?

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1519,5 @@
>      }
>  }
>  
> +void
> +nsHttpTransaction::UpdateCallbacks(nsIInterfaceRequestor* aCallbacks)

This has to be called SetSecurityCallbacks and be close to GetSecurityCallbacks definition/declaration.

@@ +1525,5 @@
> +    if (mConnection) {
> +        mConnection->SetSecurityCallbacks(aCallbacks);
> +    }
> +    MutexAutoLock lock(mCallbacksLock);
> +    mCallbacks = aCallbacks;

And what about the target?
Attachment #676825 - Flags: review?(honzab.moz) → review-
(In reply to Honza Bambas (:mayhemer) from comment #16)
> Comment on attachment 676825 [details] [diff] [review]
> @@ +1583,5 @@
> > +void
> > +nsHttpConnection::SetSecurityCallbacks(nsIInterfaceRequestor* aCallbacks)
> > +{
> > +    MutexAutoLock lock(mCallbacksLock);
> > +    mCallbacks = aCallbacks;
> 
> And what about the target?
> 
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> This has to be called SetSecurityCallbacks and be close to
> GetSecurityCallbacks definition/declaration.
> 
> @@ +1525,5 @@
> > +    if (mConnection) {
> > +        mConnection->SetSecurityCallbacks(aCallbacks);
> > +    }
> > +    MutexAutoLock lock(mCallbacksLock);
> > +    mCallbacks = aCallbacks;
> 
> And what about the target?

I don't understand these questions. Are you saying that I need to do the same proxy release dance here?
(In reply to Josh Matthews [:jdm] from comment #17)
> I don't understand these questions. Are you saying that I need to do the
> same proxy release dance here?

Yes, you need to release current callbacks on its target thread and also pass new target here to this method and save in the member as well.
Attachment #676825 - Attachment is obsolete: true
Comment on attachment 677542 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Review of attachment 677542 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping r this time rather then r-'ing.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1021,5 @@
> +void
> +nsHttpConnection::SetSecurityCallbacks(nsIInterfaceRequestor* aCallbacks,
> +                                       nsIEventTarget* aCallbackTarget)
> +{
> +    nsCOMPtr<nsIInterfaceRequestor> callbacks = aCallbacks;

Please move this line just above mCallbacks.swap(callbacks);

@@ +1586,3 @@
>      if (mCallbacks)
>          return mCallbacks->GetInterface(iid, result);
>      return NS_ERROR_NO_INTERFACE;

You didn't address my comment about not calling GetInterface under the lock.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +404,5 @@
> +{
> +    {
> +        MutexAutoLock lock(mCallbacksLock);
> +        mCallbacks = aCallbacks;
> +        mConsumerTarget = aCallbackTarget;

I'm afraid you have to do the mConsumerTarget dance here as well.

I'm thinking of having some class that we could wrap callbacks into and that would care about releasing them on the proper thread automatically.  Passing |target| argument everywhere in the API and do the release manually more and more looks like an overkill to me and also leads to mistakes like these.

Josh, up to you to introduce something lake that in this bug or in a new bug.

I think the class would be simple to implement, tho, and your patch would really simplify.  Maybe check if we could just inject this logic to the existing aggregation class?
Attachment #677542 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #20)
> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +404,5 @@
> > +{
> > +    {
> > +        MutexAutoLock lock(mCallbacksLock);
> > +        mCallbacks = aCallbacks;
> > +        mConsumerTarget = aCallbackTarget;
> 
> I'm afraid you have to do the mConsumerTarget dance here as well.
> 
> I'm thinking of having some class that we could wrap callbacks into and that
> would care about releasing them on the proper thread automatically.  Passing
> |target| argument everywhere in the API and do the release manually more and
> more looks like an overkill to me and also leads to mistakes like these.
> 
> Josh, up to you to introduce something lake that in this bug or in a new bug.
> 
> I think the class would be simple to implement, tho, and your patch would
> really simplify.  Maybe check if we could just inject this logic to the
> existing aggregation class?

I don't understand this request; no proxy releases occur anywhere in the nsHttpTransaction code right now. Is that an existing problem, or is this situation special for some reason?
(In reply to Josh Matthews [:jdm] from comment #21)
> I don't understand this request; 

I think the target releasing done this way is growing over our heads.

I want to make it more automatic and encapsulated.

We should have a wrapper class that releases callbacks on the proper thread and stop passing the target thread reference everywhere in out APIs (however internal they are) and manually calling NS_ProxyRelease().  Maybe even introduce an XPCOM (or mfbt) helper class doing that.

> no proxy releases occur anywhere in the
> nsHttpTransaction code right now. 

Sorry, it really isn't.  You just assign it at Init() where mCallbacks are always null.

But feel free to persuade me otherwise with some code reference.
Here's what I've come up with. Tests pass, and browsing appeared to work (including actions that would update callbacks).
Attachment #679396 - Flags: review?(honzab.moz)
Comment on attachment 679396 [details] [diff] [review]
Wrap up interface aggregator callbacks with a target thread on which they should be released.

Review of attachment 679396 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsInterfaceRequestorAgg.cpp
@@ +21,5 @@
> +    , mSecond(aSecond)
> +    , mConsumerTarget(aConsumerTarget)
> +  {
> +    if (!mConsumerTarget) {
> +      mConsumerTarget = NS_GetCurrentThread();;

In the .h file you say that the aConsumerTarger=null version will be released on the main thread. But this looks more like "on the thread that it was created on"?
Yeah, the documentation is incorrect.
(In reply to Josh Matthews [:jdm] from comment #23)
> Created attachment 679396 [details] [diff] [review]
> Wrap up interface aggregator callbacks with a target thread on which they
> should be released.

Any try run you could refer?
Comment on attachment 680348 [details] [diff] [review]
[FOR REFERENCE] Merge of attachemnt 677542 and attachemnt 679396 by Josh Matthews

Review of attachment 680348 [details] [diff] [review]:
-----------------------------------------------------------------

Very promising!  Thanks!

Few details and I believe we are done.

::: netwerk/protocol/http/NullHttpTransaction.cpp
@@ +27,5 @@
>  }
>  
>  NullHttpTransaction::~NullHttpTransaction()
>  {
> +  mCallbacks = nullptr;

I think you can omit this line.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +77,5 @@
>  nsHttpConnection::~nsHttpConnection()
>  {
>      LOG(("Destroying nsHttpConnection @%x\n", this));
>  
> +    mCallbacks = nullptr;

You can omit this line.

@@ +1565,2 @@
>      if (mCallbacks)
>          return mCallbacks->GetInterface(iid, result);

Please fix this in the next version.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +321,5 @@
>  
> +    // Wrap up the callbacks and the target to ensure they're released on the target
> +    // thread properly.
> +    nsCOMPtr<nsIInterfaceRequestor> wrappedCallbacks;
> +    NS_NewInterfaceRequestorAggregation(callbacks, nullptr, target, getter_AddRefs(wrappedCallbacks));

Just remove |target| all from the SpeculativeConnect APIs.  It's just another metastasis...

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +401,5 @@
> +{
> +    {
> +        MutexAutoLock lock(mCallbacksLock);
> +        mCallbacks = aCallbacks;
> +        mConsumerTarget = aCallbackTarget;

Something tells me you shouldn't do this... (update the target.)

We may start doing this when there are issues.  But still, the true consumer of the transaction is the channel that opened it, this is isn't changing when callbacks are exchanged, no?


But this is based on the original motivation for this bug that would define under what condition we may exchange callbacks.  I actually cannot find that in the bug at all!

::: xpcom/base/nsInterfaceRequestorAgg.cpp
@@ +21,5 @@
> +    , mSecond(aSecond)
> +    , mConsumerTarget(aConsumerTarget)
> +  {
> +    if (!mConsumerTarget) {
> +      mConsumerTarget = NS_GetCurrentThread();;

Two ;;

@@ +79,5 @@
> +                                    nsIInterfaceRequestor **aResult)
> +{
> +  *aResult = new nsInterfaceRequestorAgg(aFirst, aSecond, aTarget);
> +  if (!*aResult)
> +    return NS_ERROR_OUT_OF_MEMORY;

Really? :)
Attachment #680348 - Flags: review-
Comment on attachment 679396 [details] [diff] [review]
Wrap up interface aggregator callbacks with a target thread on which they should be released.

Giving positive feedback on this interdiff.
Attachment #679396 - Flags: review?(honzab.moz) → feedback+
(In reply to Honza Bambas (:mayhemer) from comment #28)
> Comment on attachment 680348 [details] [diff] [review]
> [FOR REFERENCE] Merge of attachemnt 677542 and attachemnt 679396 by Josh
> Matthews
> 
> ::: netwerk/protocol/http/NullHttpTransaction.cpp
> @@ +27,5 @@
> >  }
> >  
> >  NullHttpTransaction::~NullHttpTransaction()
> >  {
> > +  mCallbacks = nullptr;
> 
> I think you can omit this line.
> 
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ +77,5 @@
> >  nsHttpConnection::~nsHttpConnection()
> >  {
> >      LOG(("Destroying nsHttpConnection @%x\n", this));
> >  
> > +    mCallbacks = nullptr;
> 
> You can omit this line.

These bits I added to ensure that the callbacks were released at the same time as they were in the previous code. If that's not an important guarantee, then I'm fine with removing them.

> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +401,5 @@
> > +{
> > +    {
> > +        MutexAutoLock lock(mCallbacksLock);
> > +        mCallbacks = aCallbacks;
> > +        mConsumerTarget = aCallbackTarget;
> 
> Something tells me you shouldn't do this... (update the target.)
> 
> We may start doing this when there are issues.  But still, the true consumer
> of the transaction is the channel that opened it, this is isn't changing
> when callbacks are exchanged, no?
> 
> 
> But this is based on the original motivation for this bug that would define
> under what condition we may exchange callbacks.  I actually cannot find that
> in the bug at all!

There is at least nsExternalAppHandler::RetargetLoadNotifications, which was the original reason I ran into this problem.
Attachment #677542 - Attachment is obsolete: true
Attachment #679396 - Attachment is obsolete: true
Attachment #680580 - Attachment is obsolete: true
(In reply to Josh Matthews [:jdm] from comment #30)
> These bits I added to ensure that the callbacks were released at the same
> time as they were in the previous code. If that's not an important
> guarantee, then I'm fine with removing them.

I was thinking of this too.  Maybe for safety reason leave it there then.

> There is at least nsExternalAppHandler::RetargetLoadNotifications, which was
> the original reason I ran into this problem.

And what was the exact problem?
In bug 722859 I realized that private browsing downloads were holding references to private docshells, even though the original channels were retargeted away from them. This was causing private browsing mode to not actually stop when you left it, so the downloads would not be cancelled.
Attachment #680581 - Attachment is obsolete: true
>>>  nsHttpConnection::~nsHttpConnection()
>>>  {
>>> +    mCallbacks = nullptr;
>>>
>> These I added to ensure that the callbacks were released at the same
>> time as they were in the previous code. If that's not an important
>> guarantee, then I'm fine with removing them.
>
> I was thinking of this too.  Maybe for safety reason leave it there then.

mCallbacks is a class member nsCOMPtr variable, so it will be released in the destructor.  Is the issue you're worried about that we need to release the callbacks explicitly first thing because we're worried the order of member destruction (by order of declaration in the class) is somehow not safe?
I was simply being cautious. Try didn't show any problems, so I'm happy to defer to whatever you or Honza think is best.
Comment on attachment 680623 [details] [diff] [review]
Part 2: Wrap up interface aggregator callbacks with a target thread on which they should be released.

Review of attachment 680623 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab but please fix the comment before landing.

Double check you have changed all speculativeConnect calls from JS.

::: netwerk/base/public/nsISpeculativeConnect.idl
@@ +10,2 @@
>  
>  [scriptable, uuid(b3c53863-1313-480a-90a2-5b0da651ee5e)]

Change IID

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +119,5 @@
>  nsHttpTransaction::~nsHttpTransaction()
>  {
>      LOG(("Destroying nsHttpTransaction @%x\n", this));
>  
> +    mCallbacks = nullptr;

Maybe add a comment here why you do that.

::: xpcom/base/nsInterfaceRequestorAgg.cpp
@@ +21,5 @@
> +    , mSecond(aSecond)
> +    , mConsumerTarget(aConsumerTarget)
> +  {
> +    if (!mConsumerTarget) {
> +      mConsumerTarget = NS_GetCurrentThread();;

Two ;;
Attachment #680623 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/mozilla-central/rev/2bc1812f87c8
https://hg.mozilla.org/mozilla-central/rev/0d391f23c422
Assignee: nobody → josh
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 812203
Depends on: 813489
Depends on: 902158
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: