1032125 - [Nuwa] Don't send message other than NuwaFork to Nuwa after it frozen

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch

We should control what message should be sent to Nuwa after it frozen.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8458616 [details] [diff] [review]
Proposed Patch

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

Hrm, I really don't think it's a good idea to have this special logic in MessageLink... But I'm going on vacation so I'll redirect review to khuey in the meantime.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So I think that we should perhaps do this in an opt build, but in a debug build assert that we don't ever discard any non-Nuwa messages, and then fix the code that sends non-Nuwa messages.  I'm concerned that there is code that will be surprised when the messages are thrown away like this.

Does that seem reasonable?

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #4)
> So I think that we should perhaps do this in an opt build, but in a debug
> build assert that we don't ever discard any non-Nuwa messages, and then fix
> the code that sends non-Nuwa messages.  I'm concerned that there is code
> that will be surprised when the messages are thrown away like this.

The patch doesn't discard non-nuwa message but makes chrome process crash when sending non-nuwa message after nuwa ready. If we assert only in debug builds, the crash could be hidden because Nuwa is disabled by default in debug builds.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Aha, yes.  I should read more closely.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8458616 [details] [diff] [review]
Proposed Patch

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

r=me

::: ipc/glue/MessageLink.cpp
@@ +17,5 @@
>  
>  #include "nsDebug.h"
>  #include "nsISupportsImpl.h"
>  #include "nsXULAppAPI.h"
> +#include "mozilla/dom/ContentParent.h"

#ifdef MOZ_NUWA_PROCESS please

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch v.2 (r=khuey)

Updated version. I will land this after fixed blocking bugs of this.

Comment 9

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Proposed Patch v.3 (r=khuey)

Moved ContentParent::NuwaReady() to patch for bug 1040017, which is a blocking bug of this and needs the function.

Comment 12

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Don't broadcast DOMStoarge's message to frozen Nuwa.

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 3: Don't send frame message to frozen Nuwa.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8478191 [details] [diff] [review]
Part 2: Don't broadcast DOMStoarge's message to frozen Nuwa.

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

Sorry for the delay here.  I was taking advantage of you being on PTO to delay reviewing this :P

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #14)
> Sorry for the delay here.  I was taking advantage of you being on PTO to
> delay reviewing this :P

It's ok. Getting an r- when not having a computer around makes me nervious. :D

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78b119af926a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f945fb33b616
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b4e5b178aa6

Comment 17

[Carsten Book [:Tomcat]]

sorry had to backout this changes for bustage in non-unified b2g builds like https://tbpl.mozilla.org/php/getParsedLog.php?id=48376439&tree=Mozilla-Inbound

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Fixed. Landing again:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b73bd6d5d9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e193cf8cb616
https://hg.mozilla.org/integration/mozilla-inbound/rev/422fd81a4118

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0b73bd6d5d9c
https://hg.mozilla.org/mozilla-central/rev/e193cf8cb616
https://hg.mozilla.org/mozilla-central/rev/422fd81a4118

Comment 20

[Robert Chira [:RobertC]]

Can we get a backout of this? It's breaking all of our automation tests.
Additional info in Bug 1070850.

Comment 21

[Carsten Book [:Tomcat]]

backedout in https://tbpl.mozilla.org/?onlyunstarred=1&rev=2a7a78a55735

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Stacktrace is at bp-cf9d11db-c836-480f-a345-565c22140922. Okay.. It seems that I missed some notifications...

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Robert, which automated test are you running when finding this failure?

Comment 24

[Robert Chira [:RobertC]]

All of the tests were failing since the error originates in our setUp() method.

You can find the tests at the following link:
https://github.com/mozilla-b2g/gaia/tree/master/tests/python/gaia-ui-tests/gaiatest/tests/functional

A more exact location:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L817

Comment 25

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I am thinking of making chrome crash only on debug builds and discarding the messages with warning messages left on optimized builds. Althought we can run many test cases to make sure this patch won't break CI, there could be some messages we didn't fix and is sent to Nuwa. Crashing b2g in this case seems impact stability too much.

What do you think, Kyle?

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Stop sending messages to frozen nuwa (v.4)

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #25)
> I am thinking of making chrome crash only on debug builds and discarding the
> messages with warning messages left on optimized builds. Althought we can
> run many test cases to make sure this patch won't break CI, there could be
> some messages we didn't fix and is sent to Nuwa. Crashing b2g in this case
> seems impact stability too much.
> 
> What do you think, Kyle?

That sounds fine.  I hope it doesn't crash my phone too much :P

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8496757 [details] [diff] [review]
Stop sending messages to frozen nuwa (v.4)

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

::: ipc/glue/MessageLink.cpp
@@ +182,5 @@
> +            break;
> +        default:
> +#ifdef DEBUG
> +            MOZ_CRASH();
> +#else

Just use MOZ_ASSERT(false).  That'll crash debug builds only, and you can skip the ifdef.

::: ipc/glue/MessageLink.h
@@ +171,5 @@
>      MessageLoop* mIOLoop;       // thread where IO happens
>      Transport::Listener* mExistingListener; // channel's previous listener
> +#ifdef MOZ_NUWA_PROCESS
> +    bool mIsToNuwaProcess;
> +#endif

This should probably be initialized in a constructor somewhere.

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #28)
> ::: ipc/glue/MessageLink.h
> @@ +171,5 @@
> >      MessageLoop* mIOLoop;       // thread where IO happens
> >      Transport::Listener* mExistingListener; // channel's previous listener
> > +#ifdef MOZ_NUWA_PROCESS
> > +    bool mIsToNuwaProcess;
> > +#endif
> 
> This should probably be initialized in a constructor somewhere.

Yup, this is initialized in init list of ProcessLink.

Comment 30

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #28)
> Comment on attachment 8496757 [details] [diff] [review]
> Stop sending messages to frozen nuwa (v.4)
> 
> Review of attachment 8496757 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/glue/MessageLink.cpp
> @@ +182,5 @@
> > +            break;
> > +        default:
> > +#ifdef DEBUG
> > +            MOZ_CRASH();
> > +#else
> 
> Just use MOZ_ASSERT(false).  That'll crash debug builds only, and you can
> skip the ifdef.

If I use MOZ_ASSERT, the log message won't be printed on optimized builds. And I just found that I missed a return so the IPC messages will be sent on opt builds, I am going to add it to the line after printing log.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ok.

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3716351194b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/061baf99aaa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/183243f732a9

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3716351194b0
https://hg.mozilla.org/mozilla-central/rev/061baf99aaa8
https://hg.mozilla.org/mozilla-central/rev/183243f732a9