Closed
Bug 1038620
Opened 10 years ago
Closed 9 years ago
Add --nested_oop option to mach test commands
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla38
People
(Reporter: kanru, Assigned: kershaw)
References
Details
(Whiteboard: [nested-oop][ft:conndevices])
Attachments
(1 file, 12 obsolete files)
21.73 KB,
patch
|
kershaw
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Whiteboard: [nested-oop][FT:Stream3]
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → kchen
Reporter | ||
Comment 1•10 years ago
|
||
Doesn't work because SpecialPowers is not fully functional in nested content process.
Assignee | ||
Comment 3•10 years ago
|
||
Hi Kan-Ru, Would you like to take a look? Thanks.
Attachment #8473431 -
Attachment is obsolete: true
Attachment #8513297 -
Flags: feedback?(kchen)
Reporter | ||
Comment 4•10 years ago
|
||
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.
Attachment #8513297 -
Flags: feedback?(kchen) → feedback+
Assignee | ||
Comment 5•10 years ago
|
||
Update the patch with nested_setup.js. It's for adding a mozbrowser iframe.
Attachment #8513297 -
Attachment is obsolete: true
Attachment #8514766 -
Flags: feedback?(kchen)
Reporter | ||
Comment 6•10 years ago
|
||
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")
Attachment #8514766 -
Flags: feedback?(kchen) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
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.
Attachment #8514766 -
Attachment is obsolete: true
Assignee | ||
Comment 8•10 years ago
|
||
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.
Attachment #8515768 -
Attachment is obsolete: true
Attachment #8517191 -
Flags: feedback?(kchen)
Reporter | ||
Comment 9•10 years ago
|
||
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?
Attachment #8517191 -
Flags: review?(ted)
Attachment #8517191 -
Flags: feedback?(kchen)
Attachment #8517191 -
Flags: feedback+
Assignee | ||
Comment 11•10 years ago
|
||
Found some bugs when running mochitest in dom/indexedDB/test and already fixed.
Attachment #8517191 -
Attachment is obsolete: true
Attachment #8517191 -
Flags: review?(ted)
Attachment #8525171 -
Flags: review?(ted)
Updated•10 years ago
|
Whiteboard: [nested-oop][FT:Stream3] → [nested-oop][ft:conndevices]
Comment 12•10 years ago
|
||
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.
Attachment #8525171 -
Flags: review?(ted) → review+
Comment 13•10 years ago
|
||
Sorry for the review lag there, our meetup in Portland was very helpful!
Flags: needinfo?(ted)
Assignee | ||
Comment 14•10 years ago
|
||
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.
Attachment #8525171 -
Attachment is obsolete: true
Attachment #8534102 -
Flags: review?(ted)
Assignee | ||
Comment 15•10 years ago
|
||
Comment 16•10 years ago
|
||
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.
Attachment #8534102 -
Flags: review?(ted) → review+
Assignee | ||
Comment 17•10 years ago
|
||
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.
Attachment #8534102 -
Attachment is obsolete: true
Attachment #8534103 -
Attachment is obsolete: true
Attachment #8535484 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9877aca65ef5
Keywords: checkin-needed
Comment 19•10 years ago
|
||
Updated•10 years ago
|
Assignee: kechang → benj
Status: NEW → ASSIGNED
Comment 20•10 years ago
|
||
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.
Assignee: benj → kechang
Comment 21•10 years ago
|
||
(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 :)
Flags: needinfo?(kechang)
Assignee | ||
Comment 22•10 years ago
|
||
I am working on this right now. Hope to fix asap.
Flags: needinfo?(kechang)
Assignee | ||
Comment 23•9 years ago
|
||
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
Attachment #8535484 -
Attachment is obsolete: true
Attachment #8535581 -
Attachment is obsolete: true
Assignee | ||
Comment 24•9 years ago
|
||
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
Attachment #8541495 -
Attachment is obsolete: true
Attachment #8547279 -
Flags: review?(ted)
Comment 25•9 years ago
|
||
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.
Attachment #8547279 -
Flags: review?(ted) → review+
Assignee | ||
Comment 26•9 years ago
|
||
(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
Assignee | ||
Updated•9 years ago
|
Summary: Add --nested-oop option to mach test commands → Add --nested_oop option to mach test commands
Assignee | ||
Comment 27•9 years ago
|
||
Attachment #8547279 -
Attachment is obsolete: true
Attachment #8548077 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 28•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f397b3559467
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f397b3559467
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 30•9 years ago
|
||
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)
Flags: needinfo?(bugmail.mozilla)
Comment 31•9 years ago
|
||
Kershaw: it's probably better to file a new bug to track that issue.
Comment 32•9 years ago
|
||
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?
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 33•9 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•