Avoid false alarm in test_classify_ping.html

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hchang, Assigned: hchang)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/toolkit/components/url-classifier/tests/mochitest/test_classify_ping.html#41

In this test we rely on the assumption that 200ms is enough for
"ping" to reach the XHR server and it might cause both false positive
and false negative. 

We will never be aware of the false negative from the test result
but the Bug 1325054 [1] hit false positive. It doesn't directly 
imply 200ms is too short. Instead, it means 200ms is too short for
"doing local lookup and reaching the server". Does Bug 1325054 increase
the local lookup time? I cannot say "no" but it should be ignorable.
(Both are a round trip between main thread and the worker thread.)
So what change Bug 1325054 does causes the false alarm? The async call.

nsHttpChannel::BeginConnect used to *synchronously* do the local lookup
before making actual connection but Bug 1325054 makes the lookup async.
In other words, the connection will be made in the next tick no matter
how fast the lookup is. 

In the test case, we make connection for ping at [2] and verify in 200ms.
Let's assume the local lookup takes 5 seconds. For a sync lookup, the
verification will be done at least in 5 seconds since the sync lookup
occurs on the main thread and it will block the timeout handler. Also,
nsHttpChannel::BeginConnect will be called after the ping() returns
so what happened on main thread is:

- ping()
- setTimeout()
- nsHttpChannel::BeginConnect()
    - local lookup (takes 5 seconds) 
    - make connection
- timeout handler fired. Verify!

It appears 200ms doesn't include the lookup time.

However, for the async lookup:

- ping()
- setTimeout()
- nsHttpChannel::BeginConnect()
    - async local lookup (takes 5 seconds) 
- timeout handler fired. Verify!
- Async local lookup done. Making connection

The 200ms includes the lookup time.

I don't have a magic value for how long the local lookup would take.
Given that I didn't find any acknowledgement (from DOM's perspective) 
that the connection has really been made for a ping [3], What I can only 
do is retry. (unfortunately...)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1325054#c37
[2] http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/toolkit/components/url-classifier/tests/mochitest/test_classify_ping.html#37
[3] http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/docshell/base/nsDocShell.cpp#509
Assignee: nobody → hchang
Attachment #8848845 - Flags: review?(francois)
Blocks: 1325054
Comment on attachment 8848845 [details]
Bug 1348626 - Retry when isPinged() failed to avoid false alarm.

https://reviewboard.mozilla.org/r/121728/#review123774
Attachment #8848845 - Flags: review?(francois) → review+
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7541b903b15
Retry when isPinged() failed to avoid false alarm. r=francois
https://hg.mozilla.org/mozilla-central/rev/f7541b903b15
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.