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

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jerry, Assigned: cyu)

Tracking

unspecified
mozilla39
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ 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().
Attachment #8577174 - Attachment is patch: true
Comment on attachment 8577174 [details] [diff] [review]
The counter proposal: reset transports with NuwaAddConstructor()

Can we get this reviewed instead of just feedback please?
Attachment #8577174 - Flags: review?(tlee)
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.
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.
(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.
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 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.
Attachment #8577174 - Flags: review?(tlee)
Attachment #8577174 - Flags: review+
Attachment #8577174 - Flags: feedback?(tlee)
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
Assignee: nobody → cyu
Status: NEW → ASSIGNED
Keywords: checkin-needed
(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.
https://hg.mozilla.org/mozilla-central/rev/546a273cc823
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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
Attachment #8578410 - Flags: approval-mozilla-b2g37?
blocking-b2g: --- → 2.2+
Attachment #8578410 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.