Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Description • 10 years ago

Nuwa should not receive messages except messages used to controls Nuwa.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 3 • 10 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 5 • 10 years ago

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.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 6 • 10 years ago

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.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 7 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6008e82204

	Ed Morley [:emorley]
	Comment 8 • 10 years ago

Reverted for B2G nonunified build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=46451242&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=46451471&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/548e9d7175ec

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 9 • 10 years ago

Move definition of ContentParent::NuwaReinitializeData into mozilla::dom::namespace.

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 10 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/a9c2b0b7adac

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 12 • 10 years ago

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.

	Dave Hylands [:dhylands]
	Comment 13 • 10 years ago

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.

	Dave Hylands [:dhylands]
	Comment 14 • 10 years ago

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

	Patrick Wang (Chih-Kai Wang) (:kk1fff) Assignee
	Comment 15 • 10 years ago

(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.