578276 - websocket test suite is pretty flaky

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Robert Sayre]

ever since I imported 

http://hg.mozilla.org/tracemonkey/rev/29e8ddc0229f

into tracemonkey, we've been getting websocket test failures on linux and windows. I don't understand what this test is trying to accomplish, but relying on GC seems like a recipe for a flaky test. Why is it doing that? I've noticed there are several open bugs on this file for flakiness.

I'm going to disable the bit that is biting us right now, and leave most of the test in there.

Comment 1

[Robert Sayre]

I commented out the problematic line here:

http://hg.mozilla.org/tracemonkey/rev/7ef24a84b84c

On our tinderbox, the ws._testNumber field comes up undefined. I'm having problems identifying where the WebSocket object is coming from.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #0)
but relying
> on GC seems like a recipe for a flaky test. Why is it doing that? 
Because we need to test that in some cases gc doesn't kill websocket.

I've noticed
> there are several open bugs on this file for flakiness.
There are? Which ones. As far as I know, those are all fixed on m-c, 
and in fact I just marked those ones which haven't happened after fixing
Bug 572975 as fixed.

> I'm going to disable the bit that is biting us right now, and leave most of the
> test in there.
Sounds like there is are some gc related regressions in tracemonkey.
Or that tracemonkey misses some websocket patches or something.

Comment 3

[Brendan Eich [:brendan]]

How does the test depend on GC? That's generally a bad idea (design flaw). GC may finalize unpredictably. There shouldn't be any particular schedule required by a test.

/be

Comment 4

[Robert Sayre]

(In reply to comment #2)
>
> Sounds like there is are some gc related regressions in tracemonkey.
> Or that tracemonkey misses some websocket patches or something.

Oh, it definitely could be the case. Or, we may have changed things on purpose that break this test in some way. I am having trouble figuring out what the test is testing, or which WebSocket object is failing. It seems to happen near the end of the suite.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Ok, I think I know what the problem is.
Investigating (though I'm sort of on vacation/recovering from jetlag for few days).

Comment 6

[Wellington Fernando de Macedo]

> On our tinderbox, the ws._testNumber field comes up undefined. I'm having
> problems identifying where the WebSocket object is coming from.
Oh. The tests set ws._testNumber when calling the internal js CreateTestWS function. However they aren't setting that when creating the WebSocket directly. That is wrong.

Comment 7

[Wellington Fernando de Macedo]

Attachment: v1

Also, I've uploaded this in the Try-Server right now.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 457229 [details] [diff] [review]
v1

>+  PRBool HasOutgoingNotSentMessages()
>+  { return mOutgoingMessages.GetSize() != 0; }
Maybe call the method just HasOutgoingMessages()

>+      case nsIWebSocket::CLOSED:
>+      {
>+        if (!mTriggeredCloseEvent) {
>+          shouldKeepAlive = PR_TRUE;
>+        }
This could be
shouldKeepAlive = !mTriggeredCloseEvent;


I'm not 100% sure whether we need to keep the connection
alive just because of error event listeners.

Comment 9

[Wellington Fernando de Macedo]

> I'm not 100% sure whether we need to keep the connection
> alive just because of error event listeners.
Neither do I. Well, I will take it off.

Comment 10

[Wellington Fernando de Macedo]

Attachment: v2

I got this strange error on Windows in the Try-Server:

30281 ERROR TEST-UNEXPECTED-FAIL |
/tests/content/base/test/test_ws_basic_tests.html | [SimpleTest/SimpleTest.js,
window.onerror] An error occurred - shouldCloseCleanly is not defined at
http://mochi.test:8888/tests/content/base/test/test_websocket.html:553

I actually don't know why shouldCloseCleanly was undefined there (in test 21,
inside the onclose handler, and only on Windows). Well, I found out that the
previous patch made tests 21 and 22 being called twice. So I fixed that.

Anyway, I've pushed again this new patch into the Try-Server.

Comment 11

[Wellington Fernando de Macedo]

Tests succeeded in this one.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Bah, I forgot to push this before b2.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/1a2c67c52b1b