If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Rework Mozmill's sleep/waitFor methods to not spin the event loop

RESOLVED WONTFIX

Status

Testing Graveyard
Mozmill
RESOLVED WONTFIX
5 years ago
a year ago

People

(Reporter: whimboo, Unassigned)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
At the moment bug 789773 is causing us real pain because it end up in an application disconnect failure for Mozmill. Not sure if it is the zombie process which is getting created or something else in handling compartments but at some point the execution stops inside the thread.processNextEvent() call.

To give you some more details on that: In Mozmill we synchronize test steps and therefore we need methods like sleep() or waitFor() which let our code wait until a specified condition has been set. The code looks like:

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/stdlib/utils.js#L328

function sleep(milliseconds) {
  var timeup = false;

  hwindow.setTimeout(function () { timeup = true; }, milliseconds);
  var thread = Cc["@mozilla.org/thread-manager;1"]
               .getService().currentThread;
  while (!timeup) {
    thread.processNextEvent(true);
  }
}

As I have heard from Boris and Gavin a couple of months ago it's probably not a good way to do. So I'm seeking out now for advise how we can make this better.

I hope that with such a change we also have a chance to get the application disconnect sorted out. I would appreciate the feedback from each of you! Thanks!
It's worth keeping in mind that bug 789773 manifests in a zombie state because our underlying C++ appshell code actually spins the event loop while creating a top-level window: http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.cpp#1776

So the problem there definitely isn't MozMill's fault. That being said, spinning the event loop is a nasty thing to do, especially in JS. Can you rework your synchronization code to use callback functions instead?
(Reporter)

Comment 2

5 years ago
Created attachment 662074 [details]
minimized testcase

This is a simplified testcase which can be used to reproduce the hang in processNextEvent() with Firefox 16.0 Beta 3.
(Reporter)

Comment 3

5 years ago
I have talked with Bobby and Justin on IRC and what I got back is that Mozmill should stop spinning the event loop entirely (as already mentioned in the summary) and make use of callbacks instead. But that means that our test code will NO longer by synchronous and introduces all the complexity as what we know from other test frameworks like Mochitest. While this is probably the necessary way to go it will completely change the test framework.

Such a change is not something we can do in a short term. It also would have to be done wisely. So I will put this up as a roundtable item for next weeks Automation Development meeting.
Whiteboard: [mozmill-2.0?][mozmill-1.5-next?]
(Reporter)

Comment 4

5 years ago
(In reply to Bobby Holley (:bholley) from comment #1)
> It's worth keeping in mind that bug 789773 manifests in a zombie state
> because our underlying C++ appshell code actually spins the event loop while
> creating a top-level window:
> http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsXULWindow.
> cpp#1776

In case of that I will lower the severity of this bug to normal.
Severity: critical → normal
(Reporter)

Updated

5 years ago
No longer blocks: 790218
Maybe generators would do the trick? I think sticking a couple of |yield| statements into the mozmill tests would be a lot less confusing that callbacks. What do you think, Henrik?
(Reporter)

Comment 6

5 years ago
Would you mind to give an example? So far I have never used generators and I can't see right now how it would solve the problem. So I would really appreciate to hear more about your idea.
So, my understand is that mozmill wants to be able to write tests that look procedural even though they're waiting for asynchronous events. So, suppose you have a function like this:

function foo() {
  doSomething();
  spinTheEventLoopUntilX();
  doSomethingElse();
  spinTheEventLoopUntilY();
  finishUp();
}

you could instead do:

function foo() {
  doSomething();

  yield function(callback) { addListenerForX(callback); }

  doSomethingElse();

  yield function(callback) { addListenerForY(callback); }

  finishUp();
  
  // Throws StopIteration
  return;
}

Then, to invoke foo, you do:

var curr = foo;
function doStep() {
  try {
    curr()(doStep);
  } catch(e) { allDone(); }
}



Just a rough sketch - but does that give you an idea? See https://developer.mozilla.org/en-US/docs/JavaScript/Guide/Iterators_and_Generators for more details
(Reporter)

Comment 8

5 years ago
That also looks complicated but most of it could somewhat be hidden in the Mozmill API. I haven't had the time yet to check all this.

Beside that I have found the following post about os.file:
http://dutherenverseauborddelatable.wordpress.com/2012/10/03/asynchronous-file-io-for-the-mozilla-platform/

They are doing file i/o asynchronously but in the example code it looks like they are synchronizing the code again. But same here, I haven't had time to check that.
(Reporter)

Updated

5 years ago
Assignee: hskupin → nobody
Status: ASSIGNED → NEW
(Reporter)

Comment 9

5 years ago
Would be something we can do together with the move to Marionette in the not yet specified future.
Whiteboard: [mozmill-future]
(Reporter)

Comment 10

5 years ago
Also see task.js as part of the following post:
http://dutherenverseauborddelatable.wordpress.com/2012/10/18/beautiful-off-main-thread-file-io/
Sorry for bugspam.


Some weeks ago when I did a little googling about yield,
I found a lot of pages, blogs, wikis.
I would like to share them.


Related to workers:
-------------------
- https://javascriptweblog.wordpress.com/2010/06/21/web-workers-vs-the-crazy-flies/
-> http://java-script.limewebs.com/demos/swarm/buzz.html

- http://ejohn.org/blog/web-workers/

- http://xebee.xebia.in/2010/11/02/multithreading-in-javascript-with-web-workers/


Related to yield:
-----------------
- http://blog.monstuff.com/archives/000315.html

- www.neilmix.com/2007/02/07/threading-in-javascript-17/

- http://taskjs.org/

- http://hyperstruct.net/2008/5/27/synchronous-invocation-in-javascript-part-1-problem-and-basic-solution/

- https://github.com/bard/xmpp4moz/blob/4cb392d69ad889337f871d6bfb77d657ccb87076/modules/task.jsm

- http://unixpapa.com/js/sleep.html



Task.js and promises:
---------------------
- https://github.com/jvillalobos/imgur-Command/blob/master/src/bootstrap.js
-> https://github.com/mozilla/gcli/blob/master/docs/writing-commands.md
-> https://bugzilla.mozilla.org/show_bug.cgi?id=708984 Promote Promise.jsm from developer tools to toolkit
--> https://gist.github.com/2763402  Experimenting with promises.
--> https://addons.mozilla.org/en-US/developers/docs/sdk/latest/packages/api-utils/promise.html promise - Addon-SDK

- https://bugzilla.mozilla.org/show_bug.cgi?id=763311 Implement basic "Task.js" interfaces in Toolkit

- https://bugzilla.mozilla.org/show_bug.cgi?id=774816 Make the Promise module available to Toolkit consumers

- http://calculist.org/blog/2011/12/14/why-coroutines-wont-work-on-the-web/
- https://blogs.msdn.com/b/ie/archive/2011/09/11/asynchronous-programming-in-javascript-with-promises.aspx

- https://gist.github.com/3085144 createPromise() -- use promises with libraries that only support callback arguments
(Reporter)

Updated

4 years ago
Whiteboard: [mozmill-future] → [mozmill-2.2?]
(Reporter)

Comment 12

3 years ago
Mozmill will reach its end of life soon. We are currently working on getting all the tests for Firefox ported to Marionette. For status updates please see bug 1080766.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Whiteboard: [mozmill-2.2?]
(Assignee)

Updated

a year ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.