1254730 - Intermittent test_websocket1.html,test_websocket2.html,test_websocket3.html,test_websocket4.html | Test timed out. || test 34 custom server code | test 34 custom server reason

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

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

https://treeherder.mozilla.org/logviewer.html#?job_id=23222139&repo=mozilla-inbound

Comment 1

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

https://treeherder.mozilla.org/logviewer.html#?job_id=23252125&repo=mozilla-inbound

Comment 2

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

https://treeherder.mozilla.org/logviewer.html#?job_id=23279784&repo=mozilla-inbound

Comment 3

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

https://treeherder.mozilla.org/logviewer.html#?job_id=23284391&repo=mozilla-inbound

Comment 4

[Andrea Marchesini [:baku]]

Attachment: mutex.patch

The reason why we have this intermittent failure is that WebSocketChannel uses ChannelEventQueue from different threads. With this patch, I made ChannelEventQueue thread-safe, basically adding a mutex, removing ShouldEnqueue+Enqueue and adding RunOrEnqueue().

RunOrEnqueue() uses a mutex to check if we can run the ChannelEvent immediately or if we have to store it until a pending operation is completed.

This patch fixes another 'potential' error: PrependEvents() adds new events in the queue, but what about if this is called by a callback scheduled by ForceFlush()? Without this patch we use an index to schedule callbacks in order, but if we change the array in the meantime, that index points to wrong callbacks.

Comment 5

[Intermittent Failures Robot]

20 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 15
* fx-team: 3
* mozilla-central: 2

Platform breakdown:
* windows7-32: 13
* osx-10-10: 7

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1254730&startday=2016-03-07&endday=2016-03-13&tree=all

Comment 6

[Michal Novotny [:michal]]

Comment on attachment 8730005 [details] [diff] [review]
mutex.patch

Review of attachment 8730005 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r+ when you keep somehow the assertions.

::: netwerk/ipc/ChannelEventQueue.h
@@ +206,3 @@
>  }
>  
> +// Ensures that RunOrEnqueue() will collecting events during its lifetime

will be collecting

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ -382,5 @@
> -  if (mEventQ->ShouldEnqueue()) {
> -    mEventQ->Enqueue(
> -      new FTPDataAvailableEvent(this, channelStatus, data, offset, count));
> -  } else {
> -    MOZ_RELEASE_ASSERT(!mDivertingToParent,

We're losing this check in the new code (also at other places in this patch). Given that it is release assert I assume it's important and should be kept. Maybe RunOrEnqueue could have an argument assertWhenNotQueued or something like that.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/db5ef9e43187

Comment 8

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

Is this something we need on other branches too?

Comment 9

[Michal Novotny [:michal]]

> +  // @param aCallback - the ChannelEvent
> +  // @param aAssertWhenNotQueued - this optional param will be used in an
> +  //   assertion when the event is executed directly.
> +  inline void RunOrEnqueue(ChannelEvent* aCallback,
> +                           bool aAssertWhenNotQueued = true);

Description of the argument and the logic is correct, but the from the name I would expect that if the argument is true then the assertion is raised if the event is not queued. Maybe aAssertionWhenNotQueued would be better name?

And all the places below should IMO use the assertion argument as well.

> -  MOZ_RELEASE_ASSERT(mEventQ->ShouldEnqueue());
> -
> -  mEventQ->Enqueue(new HttpFlushedForDiversionEvent(this));
> +
> +  mEventQ->RunOrEnqueue(new HttpFlushedForDiversionEvent(this));

 
> -  if (mEventQ->ShouldEnqueue()) {
> -    mEventQ->Enqueue(new FTPFlushedForDiversionEvent(this));
> -  } else {
> -    MOZ_CRASH();
> -  }
> +  mEventQ->RunOrEnqueue(new FTPFlushedForDiversionEvent(this));


> -  if (mEventQ->ShouldEnqueue()) {
> -    mEventQ->Enqueue(new StopRequestEvent(this, channelStatus, timing));
> -  } else {
> -    MOZ_ASSERT(!mDivertingToParent, "ShouldEnqueue when diverting to parent!");
> -
> -    OnStopRequest(channelStatus, timing);
> -  }
> +  mEventQ->RunOrEnqueue(new StopRequestEvent(this, channelStatus, timing));

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8ae52b3b96c

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db5ef9e43187
https://hg.mozilla.org/mozilla-central/rev/a8ae52b3b96c

Comment 12

[Andrea Marchesini [:baku]]

Comment on attachment 8730005 [details] [diff] [review]
mutex.patch

Approval Request Comment
[Feature/regressing bug #]: WebSocket in workers
[User impact if declined]: a race condition can block the scheduling of events.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: The patch makes ChannelEventQueue a thread-safe object. It's a small patch but it uses mutex.
[String/UUID change made/needed]: none

This approval request is for both m-i commits.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Wes, Carsten: Can you please confirm whether this fixes the intermittent or not? It's a pretty big code change and I want to be sure this helps the situation. Thanks!

Comment 14

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

Possibly too soon to know for sure, but there's been less occurrences since this landed: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1254730

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8730005 [details] [diff] [review]
mutex.patch

This seems to be helping the intermittent failure, Aurora47+

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4ff247b3d6e8
https://hg.mozilla.org/releases/mozilla-aurora/rev/44d8ad371180

Comment 17

[Intermittent Failures Robot]

11 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* fx-team: 5
* mozilla-inbound: 3
* mozilla-central: 2
* ash: 1

Platform breakdown:
* windows7-32: 7
* osx-10-10: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1254730&startday=2016-03-14&endday=2016-03-20&tree=all