59 bytes, text/x-review-board-request
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  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  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 , What I can only do is retry. (unfortunately...)  https://bugzilla.mozilla.org/show_bug.cgi?id=1325054#c37  http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/toolkit/components/url-classifier/tests/mochitest/test_classify_ping.html#37  http://searchfox.org/mozilla-central/rev/c9f5cc6b4593d461e87a6221dc0286b3859fd515/docshell/base/nsDocShell.cpp#509
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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/f7541b903b15 Retry when isPinged() failed to avoid false alarm. r=francois
You need to log in before you can comment on or make changes to this bug.