56763 - With fast machines and slow connections, some pages timeout immediately

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Mike Kaply [:mkaply]]

We are seeing a problem on OS/2 with fast machines on slow networks 
that we believe is actually an XP problem. The problem is an immediate 
connection error.

We have found that this code in nsSocketTransport.cpp is the problem:

    idleInterval = aCurrentTime - mLastActiveTime;

We have found cases where mLastActiveTime is actually greater than 
aCurrentTime. It seems to be related to the PR_IntervalTimer, but our 
code in NSPR is the same as Windows.

We propose this code:

    if (aCurrentTime > mLastActiveTime) {
       idleInterval = aCurrentTime - mLastActiveTime;
    } else {
       idleInterval = 0;
    } /* endif */

Again, I believe this problem could occur on other platforms.

We will continue to research OS/2s PR timers, but we think this is a 
good fix for all platforms.

Comment 1

[Mike Kaply [:mkaply]]

We have more information.

We definitely believe this is an XP race condition.

The machine in question was a dual processor system.

These values actually could be out of sync.

The check we added should be there to prevent a race condition.

Comment 2

[Scott MacGregor]

Michael, did your proposed change actually fix the problem for you?

This seems like a pretty bad problem. I'm going to add it to the rtm radar.
Hopefully it isn't too late.

The proposed change seems harmless to me since we never want a negative idelTime
anyway and that's what you are protecting against....What do you think gagan?

Comment 3

[Gagan]

I totally agree. Nominating with [rtm need info] 

Comment 4

[Mike Kaply [:mkaply]]

Yup, this definitely fixed the problem.

We got it recreated on someone's machine at a conference in Germany and were 
able to debug it, and the fix worked like a charm.

They were very happy.

Comment 5

[Mike Kaply [:mkaply]]

Can I at least get an r= to put this one on the trunk?

Comment 6

[Gagan]

r=gagan

Comment 7

[Scott MacGregor]

sr=mscott

Comment 8

[Gagan]

cool. considering the small fix I hope PDT agrees for a ++... nominating.

Comment 9

[Mike Kaply [:mkaply]]

I'm changing this to be for "All" platforms to move it off the OS/2 list.

The fix is in the trunk. Hopefully it will get in the branch.

Comment 10

[Jim Roskind]

To be honest... I suspect this is a feature of OS/2.  I don't believe that
multi-processor machines should have different timers.  I guess there is a
chance that times are called on different threads... and then the difference
between them is being taken (and a multi-processor machine could wreak havoc on
such racey code).

Taking this change is *close* to taking null-pointer checks when we have no
crash scenarios... and PDT will not rtm such ungrounded bugs.

I'm going to give you all the benefit of the doubt, and run with your hunches
that this *might* be XP.  I'm grasping at straws with such an approach, but I
agree the risk is reduced to you're ability to check this in *exactly* as written.

My big hope is that this will help us with stability on multi-processor
machines... and by bigger hope is that my double plus won't land a typo regression!

RTM double plus.  Please land on branch asap.

Comment 11

[Mike Kaply [:mkaply]]

I assume that someone other than me is going to check this into the branch?

Comment 12

[Scott MacGregor]

I can if you want gagan, otherwise you can. 

Comment 13

[Scott MacGregor]

I'll take that as a yes =). I'll check into the branch right now. 

Comment 14

[Scott MacGregor]

Michael's patch was just checked into the branch. This is now fixed on the
branch and the tip. 

Comment 15

[Tom Everingham]

Michael, is there any chance you could test this on OS/2 using the branch 
build?  thx

Comment 16

[lchiang]

[e-mail] (Michael Kaply)
 - is this working ok for you?

Comment 17

[Mike Kaply [:mkaply]]

Yep. All indications from our customers is that this problem is gone.

Comment 18

[Tom Everingham]

thx

verified