1348626 - Avoid false alarm in test_classify_ping.html

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement)

Description

[Henry Chang [:hchang]]

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

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1348626 - Retry when isPinged() failed to avoid false alarm.

Review commit: https://reviewboard.mozilla.org/r/121728/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121728/

Comment 2

[François Marier [:francois]]

Comment on attachment 8848845 [details]
Bug 1348626 - Retry when isPinged() failed to avoid false alarm.

https://reviewboard.mozilla.org/r/121728/#review123774

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7541b903b15
Retry when isPinged() failed to avoid false alarm. r=francois

Comment 4

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

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