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

RESOLVED FIXED in mozilla19

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jdm, Assigned: jdm)

Tracking

Trunk
mozilla19
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

37.12 KB, patch
mayhemer
: review-
Details | Diff | Splinter Review
15.48 KB, patch
Details | Diff | Splinter Review
44.83 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 674268 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.
Attachment #674268 - Flags: review?(cbiesinger)
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+
(Assignee)

Comment 4

5 years ago
>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.
(Assignee)

Comment 6

5 years ago
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?
(Assignee)

Comment 8

5 years ago
Created attachment 676611 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

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)
(Assignee)

Updated

5 years ago
Attachment #674268 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=20dd8b41959b
(Assignee)

Comment 10

5 years ago
Created attachment 676614 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Whoops, forgot to update SetSecurityCallbacks to use the mutex.
Attachment #676614 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
Attachment #676611 - Attachment is obsolete: true
Attachment #676611 - Flags: review?(honzab.moz)
(Assignee)

Comment 11

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c81cc39d6d35
(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)
(Assignee)

Comment 14

5 years ago
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 :)
(Assignee)

Comment 15

5 years ago
Created attachment 676825 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.
Attachment #676825 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 17

5 years ago
(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.
(Assignee)

Comment 19

5 years ago
Created attachment 677542 [details] [diff] [review]
Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4e0944abec47
Attachment #677542 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
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)
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 23

5 years ago
Created attachment 679396 [details] [diff] [review]
Wrap up interface aggregator callbacks with a target thread on which they should be released.

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"?
(Assignee)

Comment 25

5 years ago
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?
Created attachment 680348 [details] [diff] [review]
[FOR REFERENCE] Merge of attachemnt 677542 and attachemnt 679396 by Josh Matthews
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+
(Assignee)

Comment 30

5 years ago
(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.
(Assignee)

Comment 31

5 years ago
Created attachment 680579 [details] [diff] [review]
Part 1: Make updating an nsHttpChannel's loadgroup/callbacks update those of its transaction and connection as well.
(Assignee)

Updated

5 years ago
Attachment #677542 - Attachment is obsolete: true
(Assignee)

Comment 32

5 years ago
Created attachment 680580 [details] [diff] [review]
Part 2: Wrap up interface aggregator callbacks with a target thread on which they should be released.
(Assignee)

Updated

5 years ago
Attachment #679396 - Attachment is obsolete: true
(Assignee)

Comment 33

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ca9ebe7252b7
(Assignee)

Comment 34

5 years ago
Created attachment 680581 [details] [diff] [review]
Part 2: Wrap up interface aggregator callbacks with a target thread on which they should be released.
(Assignee)

Updated

5 years ago
Attachment #680580 - Attachment is obsolete: true
(Assignee)

Comment 35

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=27aba4ed50b6. Ignore the last one.
(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?
(Assignee)

Comment 37

5 years ago
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.
(Assignee)

Comment 38

5 years ago
Created attachment 680623 [details] [diff] [review]
Part 2: Wrap up interface aggregator callbacks with a target thread on which they should be released.

Try run was green.
Attachment #680623 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
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?
(Assignee)

Comment 40

5 years ago
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+
(Assignee)

Comment 42

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc1812f87c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d391f23c422
https://tbpl.mozilla.org/php/getParsedLog.php?id=17038170&tree=Mozilla-Inbound#error0 has a crash that seems related to these patches.
https://hg.mozilla.org/mozilla-central/rev/2bc1812f87c8
https://hg.mozilla.org/mozilla-central/rev/0d391f23c422
Assignee: nobody → josh
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

5 years ago
Depends on: 812203

Updated

5 years ago
Depends on: 813489

Updated

3 years ago
Depends on: 902158
You need to log in before you can comment on or make changes to this bug.