The default bug view has changed. See this FAQ.

nsTransportEventSinkProxy implementation eats event notifications

RESOLVED FIXED

Status

()

Core
Networking: HTTP
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 417534 [details] [diff] [review]
v1

Http transport event sink proxy is setup to coalesce all events coming from the connection. It might lead to loose of some event notifications, often happens with STATUS_CONNECTED_TO, that is vital for successful logging. This happens because when there is still a pending event we simply do not fire (queue) a new event even it has a different status notification. It might make sense only for protocols other then http.

This bug is there from the beginning, see:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/http/src/nsHttpTransaction.cpp&rev=1.104#184
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/base/src/nsTransportUtils.cpp&rev=1.4#139

and the attachment.
Attachment #417534 - Flags: review?(bzbarsky)
1) The comment is now incorrect, yes?
2) What is the practical impact on the number and type of events delivered as a
   result of this patch?  Can you give me some statistics for some typical
   websites?
(Assignee)

Comment 2

7 years ago
(In reply to comment #1)
> 1) The comment is now incorrect, yes?

Yes

> 2) What is the practical impact on the number and type of events delivered as a
>    result of this patch? 

We get all events that can be used for logging in software like Firebug or just for correct receive of all expected chain of events when observing the transport notifications.

> Can you give me some statistics for some typical
>    websites?

I was trying to load (uncached) four web sites: www.google.cz, www.seznam.cz, www.mozilla.org and www.mozilla.com. I get 307 notification w/ this patch and 250 w/o this patch. It's about 23% raise. We have 6 states and this might indicate we were getting only 5 of them, now we get all.
What do the numbers look like on www.cnn.com?

What about gmail?
(Assignee)

Comment 4

7 years ago
(In reply to comment #3)
> What do the numbers look like on www.cnn.com?
> 

296 to 356 (~20% diff).

> What about gmail?

I unfortunately don't have a gmail account to test...
(Assignee)

Comment 5

7 years ago
And to correct the description: we do fire next event, but overwrite the current (delete it). So, we replace (coalesce) CONNECTED_TO with e.g. SENDING_TO or such an event.
> 296 to 356 (~20% diff).

Another question.  Any estimate on how long each event takes to handle?
(Assignee)

Comment 7

7 years ago
(In reply to comment #6)
> Another question.  Any estimate on how long each event takes to handle?

nsHttpChannel::OnTransportStatus: about 51ms on my 2.4 GHz Athlon64, debug build.
So 50 extra events is 2.5 seconds?  What about opt build?
(Assignee)

Comment 9

7 years ago
It will take some time to test.

What to do when there is a high performance impact with this patch? Should we leave the event sink code actually 'broken'?
Well, whether it's broken depends on what promises it makes....

If there's a high performance impact, then we should see where it's coming from.  If it's something fundamental, then perhaps we should have different modes for the events: a tracing mode for cases when we need every single event and a normal mode for what users normally need.
(Assignee)

Comment 11

7 years ago
(In reply to comment #8)
> So 50 extra events is 2.5 seconds?  What about opt build?

PerformanceCounter on windows tells me 0 for a release build, really a jump...

(In reply to comment #10)
> Well, whether it's broken depends on what promises it makes....
> 
> If there's a high performance impact, then we should see where it's coming
> from.  If it's something fundamental, then perhaps we should have different
> modes for the events: a tracing mode for cases when we need every single event
> and a normal mode for what users normally need.

We could use nsIHttpActivityObserver.isActive attribute for this, it's being checked right under the event sink initiation (coalesce = !isActive).
(Assignee)

Comment 12

7 years ago
Created attachment 419330 [details] [diff] [review]
v2

Using activity distributor isActive attribute to decide if to coalesce events or not. No impact to normal usage, developers gets all notifications as would expect.
Attachment #417534 - Attachment is obsolete: true
Attachment #419330 - Flags: review?(bzbarsky)
Attachment #417534 - Flags: review?(bzbarsky)
Comment on attachment 419330 [details] [diff] [review]
v2

>     // create transport event sink proxy that coalesces all events

This comment needs to be right above the net_NewTransportEventSinkProxy call, and probably be changed to reflect the new reality.

>+    PRBool active = PR_FALSE;

I'd actually prefer we just set active to false in the else branch here (the one where we land if NS_FAILED(rv)).

And maybe rename this to activityDistributorActive?

r=me with those changes.
Attachment #419330 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 14

7 years ago
Created attachment 423393 [details] [diff] [review]
v2.1 [Checkin comment 15]

Updated patch ready to land.
Attachment #419330 - Attachment is obsolete: true
Attachment #423393 - Flags: review+
(Assignee)

Comment 15

7 years ago
Comment on attachment 423393 [details] [diff] [review]
v2.1 [Checkin comment 15]

http://hg.mozilla.org/mozilla-central/rev/79745cb186d5
Attachment #423393 - Attachment description: v2.1 → v2.1 [Checkin comment 15]
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

7 years ago
Comment on attachment 423393 [details] [diff] [review]
v2.1 [Checkin comment 15]

For Firebug this would be nice to have also on 1.9.2.x (Fx3.6).  This is complementary fix to bug 488270 that has landed on 1.9.2.
Attachment #423393 - Flags: approval1.9.2.1?
Comment on attachment 423393 [details] [diff] [review]
v2.1 [Checkin comment 15]

Didn't make 1.9.2.2, we'll look at next time.
Attachment #423393 - Flags: approval1.9.2.2? → approval1.9.2.3?
Comment on attachment 423393 [details] [diff] [review]
v2.1 [Checkin comment 15]

Clearing old approval requests now that 1.9.2.4 has shipped. If you believe this patch is still necessary on the 1.9.2 branch please re-request approval along with a risk/benefit analysis explaining why we need it.
Attachment #423393 - Flags: approval1.9.2.4?
You need to log in before you can comment on or make changes to this bug.