Closed Bug 791960 Opened 12 years ago Closed 9 years ago

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

Categories

(Testing Graveyard :: Mozmill, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: whimboo, Unassigned)

References

Details

Attachments

(1 file)

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?
Attached file minimized testcase
This is a simplified testcase which can be used to reproduce the hang in processNextEvent() with Firefox 16.0 Beta 3.
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?]
(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
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?
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
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.
Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Would be something we can do together with the move to Marionette in the not yet specified future.
Whiteboard: [mozmill-future]
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
Whiteboard: [mozmill-future] → [mozmill-2.2?]
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
Closed: 9 years ago
Resolution: --- → WONTFIX
Whiteboard: [mozmill-2.2?]
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.