Closed Bug 1032125 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: IPC, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: kk1fff, Assigned: kk1fff)

References

Details

Attachments

(3 files, 5 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
We should control what message should be sent to Nuwa after it frozen.
Attached patch Proposed Patch (obsolete) — Splinter Review
Assignee: nobody → kk1fff
Attachment #8447902 - Attachment is obsolete: true
Attachment #8457847 - Flags: feedback?(bent.mozilla)
Depends on: 1040017
Depends on: 1040558
Depends on: 1040560
Depends on: 1040623
Attached patch Proposed Patch (obsolete) — Splinter Review
Attachment #8457847 - Attachment is obsolete: true
Attachment #8457847 - Flags: feedback?(bent.mozilla)
Attachment #8458616 - Flags: review?(bent.mozilla)
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.
Attachment #8458616 - Flags: review?(bent.mozilla) → review?(khuey)
Depends on: 1045548
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?
Flags: needinfo?(kk1fff)
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(kk1fff)
Flags: needinfo?(khuey)
Aha, yes.  I should read more closely.
Flags: needinfo?(khuey)
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
Attachment #8458616 - Flags: review+
Attached patch Proposed Patch v.2 (r=khuey) (obsolete) — Splinter Review
Updated version. I will land this after fixed blocking bugs of this.
Attachment #8458616 - Attachment is obsolete: true
Attached patch Proposed Patch v.3 (r=khuey) (obsolete) — Splinter Review
Moved ContentParent::NuwaReady() to patch for bug 1040017, which is a blocking bug of this and needs the function.
Attachment #8465357 - Attachment is obsolete: true
Duplicate of this bug: 1040560
Duplicate of this bug: 1045548
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
Attachment #8478191 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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
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
Can we get a backout of this? It's breaking all of our automation tests.
Additional info in Bug 1070850.
Status: RESOLVED → REOPENED
Flags: needinfo?(cbook)
Resolution: FIXED → ---
Blocks: 1070850
Stacktrace is at bp-cf9d11db-c836-480f-a345-565c22140922. Okay.. It seems that I missed some notifications...
Robert, which automated test are you running when finding this failure?
Flags: needinfo?(robert.chira)
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
Flags: needinfo?(robert.chira)
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?
Flags: needinfo?(khuey)
Attachment #8496757 - Attachment description: Proposed Patch v.4 → Stop sending messages to frozen nuwa (v.4)
(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
Flags: needinfo?(khuey)
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.
Attachment #8496757 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Flags: needinfo?(khuey)
Attachment #8475825 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.