1348016 - nsHttpConnectionMgr::OnMsgCancelTransactions miss to cancel one transaction

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dragana Damjanovic [:dragana]]

This is a regression from bug 1312774. I notice it while I was rebasing 1341572.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_missing_one_trans_during_canceling.patch

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8848160 [details] [diff] [review]
bug_missing_one_trans_during_canceling.patch

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -2130,5 @@
>          return;
>      }
>  
> -    for (int32_t i = ent->mUrgentStartQ.Length() - 1; i > 0;) {
> -        --i;

Thanks Dragana, but the correct fix is:


for (int32_t i = ent->mUrgentStartQ.Length(); i > 0;) {
  --i;
  ...
}

This change was reviewed, fix suggested, but apparently not implemented (sorry to miss this - important - detail during r).

Comment 3

[Dragana Damjanovic [:dragana]]

Attachment: bug_missing_one_trans_during_canceling.patch

The other version would work too, our pending queue is never that full

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8848177 [details] [diff] [review]
bug_missing_one_trans_during_canceling.patch

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

Thanks.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Dragana Damjanovic [:dragana] from comment #3)
> The other version would work too, our pending queue is never that full

If we ever migrate to uint32_t or size_t or nsTArray::index_type here, we would overflow (0 - 1 = 0xffffffff), badly when not caught.  It's just a precaution for future ;)

Comment 6

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> (In reply to Dragana Damjanovic [:dragana] from comment #3)
> > The other version would work too, our pending queue is never that full
> 
> If we ever migrate to uint32_t or size_t or nsTArray::index_type here, we
> would overflow (0 - 1 = 0xffffffff), badly when not caught.  It's just a
> precaution for future ;)

this one is fixed and 20 others are going to overflow... in case we switch we will need to audit this file anyway.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a8029b94ff
nsHttpConnectionMgr::OnMsgCancelTransactions miss to cancel one transaction. r=mayhemer

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d4a8029b94ff