If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

websocket test suite is pretty flaky

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: Robert Sayre, Assigned: Wellington Fernando de Macedo)

Tracking

unspecified
mozilla2.0b3
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
18.55 KB, patch
smaug
: review+
Benjamin Smedberg
: approval2.0+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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.
(Reporter)

Comment 1

7 years ago
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

7 years ago
(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.
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
(Reporter)

Comment 4

7 years ago
(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

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

Comment 6

7 years ago
> 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.
(Assignee)

Comment 7

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

Also, I've uploaded this in the Try-Server right now.
Assignee: nobody → wfernandom2004
Status: NEW → ASSIGNED
Attachment #457229 - Flags: review?(Olli.Pettay)

Comment 8

7 years ago
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.
Attachment #457229 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 9

7 years ago
> 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.
(Assignee)

Comment 10

7 years ago
Created attachment 457302 [details] [diff] [review]
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.
Attachment #457229 - Attachment is obsolete: true
Attachment #457302 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 11

7 years ago
Tests succeeded in this one.

Updated

7 years ago
Attachment #457302 - Flags: review?(Olli.Pettay) → review+
Bah, I forgot to push this before b2.
Keywords: checkin-needed

Updated

7 years ago
Attachment #457302 - Flags: approval2.0?

Updated

7 years ago
Attachment #457302 - Flags: approval2.0? → approval2.0+
http://hg.mozilla.org/mozilla-central/rev/1a2c67c52b1b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla2.0b3
You need to log in before you can comment on or make changes to this bug.