922465 - Automated tests for the Nuwa process

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Cervantes Yu [:cyu] [:cervantes]]

The Nuwa templated content process is now disabled by default. We need to add automated tests for turning it on.

Comment 1

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

Attachment: WIP: Observe Nuwa process creation.

Test to observe NuwaReady. This basically works if we startup emulator only for this test case, since the preallocated process is not there then, we can observe it's event. But if the emulator has already run many tests before running this, we will need to restart the preallocated process. The preference observer doesn't seem work, I am still finding a way to restart it.

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 819300 [details] [diff] [review]
WIP: Observe Nuwa process creation.

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

This test looks good, but we need to consider for the failure path and don't block the whole test suite if RecvNuwaReady() is never called.

If for some reason the Nuwa process won't be ready, we need to timeout and fail the test explicitly. The timeout value can be a loose, but not too big, value. 1 min is a safe value, I guess.

Comment 3

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

Attachment: WIP: Observe Nuwa process creation.

Comment 4

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

Current status of test case: since running program in emulator is slower than in a real device, we are likely to be affected by bug 941466. It would result in all test cases time out once Nuwa is turned on.

Cervantes suggests we turn off PreloadSlowThings in emulator prevent asynchrous events from being dispatched after start to freeze. I tested turing off PreloadSlowThings on try server, the tests cases are all passed. I think we can turn off PreloadSlowThings on emulator for now, and re-enable it after we fixed bug 941466.

Comment 5

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

I don't really like the idea of turning off PreloadSlowThings in the emulator because it means that we're no longer testing the same code that we're shipping. We have every reason to believe that users will hit this.

Comment 6

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

Attachment: Test nuwa creation and fork new process.

Comment 7

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

Comment on attachment 8340314 [details] [diff] [review]
Test nuwa creation and fork new process.

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

Kyle, could you help review this patch?

Comment 8

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

Comment on attachment 8340314 [details] [diff] [review]
Test nuwa creation and fork new process.

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

::: dom/ipc/PreallocatedProcessManager.cpp
@@ +285,5 @@
> +    nsCOMPtr<nsIMessageBroadcaster> ppmm =
> +      do_GetService("@mozilla.org/parentprocessmessagemanager;1");
> +    nsresult rv = ppmm->BroadcastAsyncMessage(
> +      NS_LITERAL_STRING("TEST-ONLY:nuwa-add-new-process"),
> +      JSVAL_NULL, JSVAL_NULL, AutoJSContext(), 1);

AutoJSContext cannot be used as a temporary.  If you ran this in a debug build it would assert at http://mxr.mozilla.org/mozilla-central/source/mfbt/GuardObjects.h#97

@@ +324,5 @@
> +    nsCOMPtr<nsIMessageBroadcaster> ppmm =
> +      do_GetService("@mozilla.org/parentprocessmessagemanager;1");
> +    nsresult rv = ppmm->BroadcastAsyncMessage(
> +      NS_LITERAL_STRING("TEST-ONLY:nuwa-ready"),
> +      JSVAL_NULL, JSVAL_NULL, AutoJSContext(), 1);

Same here.

Comment 9

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

Attachment: Patch: Test nuwa creation and fork new process.

I just realized AutoJSContext cannot be used as a temporary object. Thank for catching this, Kyle.

Comment 10

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

Attachment: Patch: Test nuwa creation and fork new process.

Remove redundant log message from test_NuwaProcessCreation.html

Comment 11

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

Comment on attachment 8343606 [details] [diff] [review]
Patch: Test nuwa creation and fork new process.

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

::: dom/ipc/tests/test_NuwaProcessCreation.html
@@ +75,5 @@
> +  };
> +  let timeout = setTimeout(function() {
> +    ok(false, "Nuwa process is not launched");
> +    testEnd();
> +  }, 60000);

Do we really need to wait a full minute here?

Comment 12

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #11)
> Do we really need to wait a full minute here?

It won't take that long if test pass. If we get the two message, we can finish the test. Setting 1 minute is because emulator is slower then real device.

Comment 13

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

https://hg.mozilla.org/integration/b2g-inbound/rev/bf8e90edd152

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/06b3a7aea2c0 for frequent (somewhere between 20 and 50%) xpcshell shutdown crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=31651189&tree=B2g-Inbound

Comment 16

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

https://hg.mozilla.org/integration/b2g-inbound/rev/d48b1a339842

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Preeti Raghunath(:Preeti)]

for rel eng

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11ec4ba33de4