1142935 - Fix channel outgoing buffer data race problem for NuwaAddFinalConstructor() callback

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jerry Shih[:jerry] (UTC+8) (inactive)]

+++ This bug was initially created as a clone of Bug #1139090 +++

As https://bugzilla.mozilla.org/show_bug.cgi?id=1139090#c41 , we have some race problem if we send ipc message in NuwaAddFinalConstructor().
We might fix that if we enqueue the hello message in NuwaAddConstructor().

Comment 1

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: The counter proposal: reset transports with NuwaAddConstructor()

move from https://bugzilla.mozilla.org/show_bug.cgi?id=1139090#c57

Comment 2

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8577174 [details] [diff] [review]
The counter proposal: reset transports with NuwaAddConstructor()

Can we get this reviewed instead of just feedback please?

Comment 3

[Jerry Shih[:jerry] (UTC+8) (inactive)]

With attachment 8577174 [details] [diff] [review], it seems good with the crash STR https://bugzilla.mozilla.org/show_bug.cgi?id=1139090#c30

I will create a try push later.

Comment 4

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Here is the try with nuwa and vsync-rd. I got a lot of M9 failed, but I also see the same failed in "similar job" field.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25d5dd741f3a

Waiting for the try without all change.

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #4)
> Here is the try with nuwa and vsync-rd. I got a lot of M9 failed, but I also
> see the same failed in "similar job" field.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=25d5dd741f3a
> 
> Waiting for the try without all change.

R9 has been perma-fail for a while  now, it's not this patch's fault.

Comment 6

[Jerry Shih[:jerry] (UTC+8) (inactive)]

This try push doesn't contain this patch and vsync-rd, but still have m9 failed. So it's not this patch failed.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b144b87d0f4b

Will discuss with Thinker again for this patch.

Comment 7

[Thinker Li [:sinker]]

Comment on attachment 8577174 [details] [diff] [review]
The counter proposal: reset transports with NuwaAddConstructor()

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

please file a follow-up bug that checks if there is any IPC activity before reset.

Comment 8

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Attachment: reset transports with NuwaAddConstructor(). r=tlee

Comment 9

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Please land the attachment 8578410 [details] [diff] [review] to m-c

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25d5dd741f3a

There are a lot of M9 failed tests, but it seems not related to this patch.
Here is the try result without this patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b144b87d0f4b

Comment 10

[Mason Chang [Inactive] [:mchang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/546a273cc823

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to Thinker Li [:sinker] from comment #7)
> Comment on attachment 8577174 [details] [diff] [review]
> The counter proposal: reset transports with NuwaAddConstructor()
> 
> Review of attachment 8577174 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> please file a follow-up bug that checks if there is any IPC activity before
> reset.

Filed as bug 1144004.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/546a273cc823

Comment 13

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Comment on attachment 8578410 [details] [diff] [review]
reset transports with NuwaAddConstructor(). r=tlee

Approval Request Comment
[Feature/regressing bug #]:
project silk, b2g crash when we turn on vsync-refresh driver(Bug 1139090)
[User impact if declined]: b2g has data race problem for nuwa and ipc system. Without this patch, b2g is easy crashed.
[Describe test coverage new/current, TreeHerder]: manual testing. I don't get any failed or crash at b2g 2.2
[Risks and why]: low. we only change the nuwa ipc channel init flow, and it just called once when nuwa is cloning.
[String/UUID change made/needed]: none

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/1e583b3818db