1038620 - Add --nested_oop option to mach test commands

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Similar to bug 932149, we want to add a --nested-oop option to mach so we could run all mochitest under a nested oop iframe.

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: WIP patch

Doesn't work because SpecialPowers is not fully functional in nested content process.

Comment 2

[Kershaw Chang [:kershaw]]

I will try to take care of this bug.

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: WIP patch

Hi Kan-Ru,

Would you like to take a look?

Thanks.

Comment 4

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8513297 [details] [diff] [review]
WIP patch

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

Looks good to me. You have some non-related whitespace change. You didn't include nested_setup.js in the patch.

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: WIP

Update the patch with nested_setup.js. 
It's for adding a mozbrowser iframe.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8514766 [details] [diff] [review]
WIP

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

::: testing/mochitest/nested_setup.js
@@ +23,5 @@
> +  iframe.width = "100%";
> +  iframe.height = "100%";
> +  iframe.scoring = "no";
> +  iframe.setAttribute("remote", "true");
> +  SpecialPowers.wrap(iframe).mozbrowser = true;

Use iframe.setAttribute("mozbrowser", "true")

Comment 7

[Kershaw Chang [:kershaw]]

Attachment: Add --nested-oop option to mach test commands - v1

Update the patch with the feedback.

Kan-Ru,

Do you think can we start the review process?
If yes, could you suggest someone in ateam to do this?

Thanks.

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Add --nested-oop option to mach test commands - v2

Hi Kan-Ru,
I've fixed some errors in previous version. Now, looks like it can run most mochitests (I just try dom/base and dom/html).
If you think it's ok for reviewing, could you help me to find a reviewer?
Thanks.

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8517191 [details] [diff] [review]
Add --nested-oop option to mach test commands - v2

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

Looks good. Ted, could you help reviewing this?

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

review ping.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Add --nested-oop option to mach test commands - v3

Found some bugs when running mochitest in dom/indexedDB/test and already fixed.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 8525171 [details] [diff] [review]
Add --nested-oop option to mach test commands - v3

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

::: testing/mochitest/mach_commands.py
@@ +510,5 @@
>      func = this_chunk(func)
>  
> +    this_chunk = CommandArgument('--nested', action='store_true',
> +        help='Run tests with nested oop preferences and test filtering enabled.')
> +    func = this_chunk(func)

It might make it clearer to name this '--nested-oop' or '--nested-process' or something like that.

::: testing/mochitest/mochitest_options.py
@@ +462,5 @@
>              options.e10s = True
>  
>          mozinfo.update({"e10s": options.e10s}) # for test manifest parsing.
>          mozinfo.update({"contentSandbox": options.contentSandbox}) # for test manifest parsing.
> +        mozinfo.update({"nested": options.nested}) # for test manifest parsing.

Similarly here, since this is going to wind up in manifests, something like "nestedoop" or "nested_e10s" would probably be clearer.

::: testing/mochitest/nested_setup.js
@@ +7,5 @@
> +    [{ type: "browser", allow: true, context: document }],
> +    addPreferences);
> +}
> +
> +function addPreferences() 

nit: trailing whitespace

::: testing/specialpowers/content/specialpowers.js
@@ +12,5 @@
> +const Ci = Components.interfaces;
> +
> +const CHILD_SCRIPT = "chrome://specialpowers/content/specialpowers.js"
> +const CHILD_SCRIPT_API = "chrome://specialpowers/content/specialpowersAPI.js"
> +const CHILD_LOGGER_SCRIPT = "chrome://specialpowers/content/MozillaLogger.js"

nit: missing semicolons on these lines.

@@ +64,5 @@
> +        mm.loadFrameScript(CHILD_LOGGER_SCRIPT, true);
> +        mm.loadFrameScript(CHILD_SCRIPT_API, true);
> +        mm.loadFrameScript(CHILD_SCRIPT, true);
> +        mm.addMessageListener("SPPrefService", function(msg) {
> +          return self._sendSyncMessage('SPPrefService', msg.data)[0];

It would be nice to figure out a way to not have to repeat yourself so many times. I'm also a little worried about the list of messages getting out of sync between here and the other SpecialPowers listener. You should at least add a comment here and in the other listener mentioning that these lists should be kept in sync.

Comment 13

[(not currently active) Ted Mielczarek]

Sorry for the review lag there, our meetup in Portland was very helpful!

Comment 14

[Kershaw Chang [:kershaw]]

Attachment: Add --nested-oop option to mach test commands - v4

Hi Ted,

Thanks for review the patch!
I've created a v4 patch with the following changes:
1. Replace --nested with --nested-oop to make it clearer.
2. Add two lists for all SP messages sent to observer. This should be helpful for syncing.

Please take a look at v4 patch. Thanks.

Comment 15

[Kershaw Chang [:kershaw]]

Attachment: interdiff for v4 and v3

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 8534102 [details] [diff] [review]
Add --nested-oop option to mach test commands - v4

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

::: testing/mochitest/mochitest_options.py
@@ +489,5 @@
>              options.e10s = True
>  
>          mozinfo.update({"e10s": options.e10s}) # for test manifest parsing.
>          mozinfo.update({"contentSandbox": options.contentSandbox}) # for test manifest parsing.
> +        mozinfo.update({"nested-oop": options.nested_oop}) # for test manifest parsing.

This needs to be "nested_oop", I just checked and our manifest expression parser only allows alphanumeric digits and underscores in identifiers.

::: testing/specialpowers/content/specialpowers.js
@@ +14,5 @@
> +const CHILD_SCRIPT = "chrome://specialpowers/content/specialpowers.js";
> +const CHILD_SCRIPT_API = "chrome://specialpowers/content/specialpowersAPI.js";
> +const CHILD_LOGGER_SCRIPT = "chrome://specialpowers/content/MozillaLogger.js";
> +
> +// All messages sent to observer should be listed here.

Thanks for this, this is nicer.

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Add --nested-oop option to mach test commands - v5, r=ted.mielczarek

1. Carry reviewer's name.
2. Rebase.
3. Reviewer's feedback is addressed.

I'll file another two bugs for getting nested-oop running on try server and getting nested-oop to pass all tests.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9877aca65ef5

Comment 19

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Add --nested-oop option to mach test commands - v5

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Oops sorry didn't intend that, I am playing with bzexport and it happens it applies to tip when there are no arguments provided. Sorry about that.

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9877aca65ef5

this had to be backedout for causing test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4584990&repo=mozilla-inbound

Please provide a try run before requesting checkin-needed again, thanks :)

Comment 22

[Kershaw Chang [:kershaw]]

I am working on this right now.
Hope to fix asap.

Comment 23

[Kershaw Chang [:kershaw]]

Attachment: Add --nested-oop option to mach test commands - v6

Update the patch that finally make treeherder happy.
This patch has the following changes:
1. Move const variable declaration inside SpecialPowers.
2. Move the "remote-browser-shown" code into another function that only excuted when nested-oop is enabled.

Test result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79863c1c5335

Comment 24

[Kershaw Chang [:kershaw]]

Attachment: Add --nested_oop option to mach test commands - v7

Hi Ted,

Could you help to review the latest patch again?
I've done the following changes:
1. Replace all "nested-oop" to "nested_oop" for consistency.
2. Add a check to make sure that --e10s must be also used with --nested_oop.

Thanks.

Test result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b683b8a975a9

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 8547279 [details] [diff] [review]
Add --nested_oop option to mach test commands - v7

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

::: testing/mochitest/mochitest_options.py
@@ +650,5 @@
>                      self.error('Missing binary %s required for --use-test-media-devices')
>  
> +        if options.nested_oop:
> +          if not options.e10s:
> +            self.error('e10s must be also enabled with nested_oop.')

This is fine, but it seems like it would also be fine to make --nested-oop imply --e10s, so that you only have to specify the one argument to make everything work.

Comment 26

[Kershaw Chang [:kershaw]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25)
> Comment on attachment 8547279 [details] [diff] [review]
> Add --nested_oop option to mach test commands - v7
> 
> Review of attachment 8547279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mochitest/mochitest_options.py
> @@ +650,5 @@
> >                      self.error('Missing binary %s required for --use-test-media-devices')
> >  
> > +        if options.nested_oop:
> > +          if not options.e10s:
> > +            self.error('e10s must be also enabled with nested_oop.')
> 
> This is fine, but it seems like it would also be fine to make --nested-oop
> imply --e10s, so that you only have to specify the one argument to make
> everything work.

Thanks. I will change it to:

         if options.nested_oop:
           if not options.e10s:
-            self.error('e10s must be also enabled with nested_oop.')
+            options.e10s = True

Comment 27

[Kershaw Chang [:kershaw]]

Attachment: Add --nested_oop option to mach test commands, r=ted.mielczarek

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f397b3559467

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 30

[Kershaw Chang [:kershaw]]

Hi kats,

I found the error below after your patch (bug 1107009) is applied when running mochitest with nested_oop option. Could you please take a look?

To run mochitest with nested_oop, you could do this: ./mach mochitest-plain --nested_oop dom/base/test/test_Image_constructor.html.

Thanks.

###!!! ERROR: Potential deadlock detected:
=== Cyclical dependency starts at
--- Mutex : IndirectLayerTree (currently acquired)
 calling context
  [stack trace unavailable]
=== Cycle completed at
--- Mutex : IndirectLayerTree (currently acquired)
 calling context
  [stack trace unavailable]
###!!! Deadlock may happen NOW!
[Parent 29928] ###!!! ASSERTION: Potential deadlock detected:
Cyclical dependency starts at
Mutex : IndirectLayerTree (currently acquired)
Cycle completed at
Mutex : IndirectLayerTree (currently acquired)
###!!! Deadlock may happen NOW!
: 'Error', file /home/kershaw/work/firefox_git/xpcom/glue/BlockingResourceBase.cpp, line 307
#01: mozilla::OffTheBooksMutex::Lock() (/home/kershaw/work/firefox_git/xpcom/glue/BlockingResourceBase.cpp:382)
#02: mozilla::layers::CompositorParent::NotifyChildCreated(unsigned long const&) (/home/kershaw/work/firefox_git/gfx/layers/ipc/CompositorParent.cpp:1259)
#03: mozilla::layers::CrossProcessCompositorParent::RecvNotifyChildCreated(unsigned long const&) (/home/kershaw/work/firefox_git/gfx/layers/ipc/CompositorParent.cpp:1645)
#04: mozilla::layers::PCompositorParent::OnMessageReceived(IPC::Message const&) (/home/kershaw/work/firefox_git/obj-x86_64-unknown-linux-gnu/ipc/ipdl/PCompositorParent.cpp:473)
#05: ~AutoSetValue (/home/kershaw/work/firefox_git/obj-x86_64-unknown-linux-gnu/ipc/glue/../../dist/include/mozilla/ipc/MessageChannel.h:490)
#06: mozilla::Maybe<mozilla::dom::AutoNoJSAPI>::reset() (/home/kershaw/work/firefox_git/obj-x86_64-unknown-linux-gnu/ipc/glue/../../dist/include/mozilla/Maybe.h:375 (discriminator 1))
#07: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (/home/kershaw/work/firefox_git/ipc/glue/MessageChannel.cpp:1129)
#08: MessageLoop::RunTask(Task*) (/home/kershaw/work/firefox_git/ipc/chromium/src/base/message_loop.cc:362)
#09: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/kershaw/work/firefox_git/ipc/chromium/src/base/message_loop.cc:372)
#10: MessageLoop::DoWork() (/home/kershaw/work/firefox_git/ipc/chromium/src/base/message_loop.cc:447)
#11: base::MessagePumpDefault::Run(base::MessagePump::Delegate*) (/home/kershaw/work/firefox_git/ipc/chromium/src/base/message_pump_default.cc:35)
#12: MessageLoop::RunInternal() (/home/kershaw/work/firefox_git/ipc/chromium/src/base/message_loop.cc:234)
#13: ~AutoRunState (/home/kershaw/work/firefox_git/ipc/chromium/src/base/message_loop.cc:508)
#14: base::Thread::ThreadMain() (/home/kershaw/work/firefox_git/ipc/chromium/src/base/thread.cc:173)
#15: ThreadFunc (/home/kershaw/work/firefox_git/ipc/chromium/src/base/platform_thread_posix.cc:41)
#16: start_thread (/build/buildd/eglibc-2.19/nptl/pthread_create.c:312 (discriminator 2))
#17: __clone (/build/buildd/eglibc-2.19/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:113)

Comment 31

[(not currently active) Ted Mielczarek]

Kershaw: it's probably better to file a new bug to track that issue.

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Yes, please do file a new bug and make it block bug 1107009. I'll look into it. Is this on Linux or a B2G emulator?

Comment 33

[Kershaw Chang [:kershaw]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #32)
> Yes, please do file a new bug and make it block bug 1107009. I'll look into
> it. Is this on Linux or a B2G emulator?

Bug 1122408 is filed for this, but I didn't make it block bug 1107009 due to lack of access right. 
I found this both on linux and mac. 

Thanks.