Closed Bug 1040017 Opened 7 years ago Closed 7 years ago

[Nuwa] Don't forward observed notification to Nuwa after Nuwa ready

Categories

(Core :: DOM: Content Processes, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: kk1fff, Assigned: kk1fff)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch Proposed Patch (obsolete) — Splinter Review
Nuwa should not receive messages except messages used to controls Nuwa.
Summary: [Nuwa] Don't send SetOffline message to Nuwa after Nuwa ready → [Nuwa] Don't forward observed notification to Nuwa after Nuwa ready
Duplicate of this bug: 1040558
Duplicate of this bug: 1040623
Attached patch Proposed Patch v2 (obsolete) — Splinter Review
This patch makes ContentParent not send observed notification to Nuwa directly. If Nuwa's ContentParent observed something that needs to be forwarded to child after nuwa ready, it stores the values and resends them to child when a new process is created.
Assignee: nobody → kk1fff
Attachment #8457911 - Attachment is obsolete: true
Attachment #8465344 - Flags: review?(khuey)
Attachment #8465344 - Flags: review?(khuey)
Attached patch Proposed Patch v3 (obsolete) — Splinter Review
Attachment #8465344 - Attachment is obsolete: true
Attachment #8466008 - Flags: review?(khuey)
Comment on attachment 8466008 [details] [diff] [review]
Proposed Patch v3

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

::: dom/ipc/ContentParent.cpp
@@ +180,5 @@
>  #ifdef MOZ_NUWA_PROCESS
>  int32_t ContentParent::sNuwaPid = 0;
>  bool ContentParent::sNuwaReady = false;
> +
> +// Contains data that is observed by Nuwa and is going to be send to

s/send/sent/

@@ +182,5 @@
>  bool ContentParent::sNuwaReady = false;
> +
> +// Contains data that is observed by Nuwa and is going to be send to
> +// the new process when it is forked.
> +struct ContentParent::NuwaReinitialzeData {

there's an 'i' between l and z in Reinitialize.

@@ +2511,5 @@
> +#ifdef MOZ_NUWA_PROCESS
> +      if (!(IsNuwaReady() && IsNuwaProcess())) {
> +#endif
> +          if (!SendSetOffline(!strcmp(offline, "true") ? true : false))
> +              return NS_ERROR_NOT_AVAILABLE;

While you're here add braces for this if().

@@ +2597,1 @@
>          unused << SendFilePathUpdate(file->mStorageType, file->mStorageName, file->mPath, creason);

I would prefer that you wrote the preprocessing conditional so that the actual method call is the same in both cases.

@@ +2645,3 @@
>          unused << SendFileSystemUpdate(volName, mountPoint, state,
>                                         mountGeneration, isMediaPresent,
>                                         isSharing, isFormatting, isFake);

here too.
Attachment #8466008 - Flags: review?(khuey) → review+
Component: IPC → DOM: Content Processes
Attached patch Patch v.4 (r=khuey) (obsolete) — Splinter Review
Update patch according to review comment. I also moved ContentParent::NuwaReady() function form patch for bug 1032125 to this patch. That patch has already been reviewed, just because this bug is one of the bugs that block its landing.
Attachment #8466008 - Attachment is obsolete: true
Attachment #8475820 - Attachment description: Patch v.4 → Patch v.4 (r=khuey)
Move definition of ContentParent::NuwaReinitializeData into mozilla::dom::namespace.
Attachment #8475820 - Attachment is obsolete: true
Build pass on try (non unified build is not on try though, I built it locally).

push: https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c2b0b7adac
https://hg.mozilla.org/mozilla-central/rev/a9c2b0b7adac
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1058376
Comment on attachment 8477179 [details] [diff] [review]
Patch v.5 (r=khuey)

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

After digging into how device storage works, I am not sure if this is a proper way to stop sending file update messages to Nuwa. This causes bugs (like bug 1058376). If this patch is not right, I'd prefer back it out and rewrite instead of providing fix based on it.
Attachment #8477179 - Flags: feedback?(dhylands)
So, I think that Nuwa can just ignore file update messages.

The purpose for the file update notifications is to tell apps that are using device storage, that a change has occurred (so an async notification).

The normal flow of an app which uses device storage (gallery, music, video - for example) is to perform an enumeration of the file system (or read a database) to get the initial state, and then use the async notifications to get changes.

Any changes prior to a child getting the initial state are essentially irrelevant.

So there is no reason for an app to get file update messages until it's actually a real app and its executed some JS to open device storage and get some initial state.

From the Nuwa perspective, I think that this means that the Nuwa process can safely ignore file update messages, and there is no need to remember any file update notifications that might come along.

Once bug 1036877 lands, then file system updates can also be ignored by Nuwa.
Comment on attachment 8477179 [details] [diff] [review]
Patch v.5 (r=khuey)

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

Clearing feedback, since I think I answered it in comment 13
Attachment #8477179 - Flags: feedback?(dhylands)
Depends on: 1058449
(In reply to Dave Hylands [:dhylands] from comment #13)

Thank you for explaining, Dave. I just tried with logs and saw that nothing is observing file update in Nuwa or in preallocated proc.
Depends on: 1063189
You need to log in before you can comment on or make changes to this bug.