567417 - [tracking] bug to get mochitests running on fennec + e10s

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

there are a lot of pieces to this, here is where we are for now:

1) build mochitest-chrome style harness for mochitest-plain
2) allow for test case listing and discovery (currently done via the http webserver, but this is problematic on my latest builds and isn't how mochitest-chrome does it)
3) define messageManager() proxy layer for handling content->chrome for core mochikit/simpletest needs (logging, timeout, testFinished, closeWindow, events like mousemove, plugin events).
4) rework some code in mochikit/simpletest that is not event driven so we can achieve two way communication.  this needs to be backward compatible
5) package up the small bits needed for the chrome harness (currently we just map to files on the hdd for mochitest-chrome, but we need to ensure we have everything in order and with the right paths.
6) resolve focus and dom manipulation from simpletest->content.  At the moment, I don't know all the issues here, just a few of them
7) identify all test specific privilege requests (like getPref, setPref, etc...) and add to messageManager library.  Rewrite tests to remove privilege escalation and be more event driven.  This needs to be backward compatible.
8) currently we have thousands of tests that fail (about 410/2750 test files).  We can remove these and just run the green tests (about 32000), or figure out how to determine if we have a e10s failure vs a fennec failure.

If this doesn't sound right, please comment in this bug.

Comment 1

[Benjamin Smedberg]

If the test case listing is done via HTTP webserver, why do we need to use the chrome-style harness instead of the content harness?

Are there particular tasks within #3 that we can help with? We have a lot of message-manager expertise floating around, though not a lot of mochitest-harness expertise. It's still not clear to me which functions of mochitest escape the content boundary in the typical case.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

I have gone back and forth on the 'chrome' style vs 'plain' harness.  To be honest I don't know the extent of the work of each, but from what I see the dividing line is cleaner when running in a 'chrome' style harness.

In regards to the test listing, I have had a lot of trouble with the webserver since it is all in js code and run via xpcshell binary.  It could be a current bug.  The general code is shared between 'chrome' and 'plain', but there are some differences.  For example in 'plain' we would need to modify the server.js file to not insert script files into the testharness window.  The server.js file also returns a variable into the webpage with a list of all the test cases.  More fuzzy water for chrome vs content process.

I think the most difficult part is turning some of the code into event driven code instead of just calling an api and getting a value returned.  If we could have a library which was synchronous that we could access from content pages that would help a lot:)  I think for file logging, closing the window and iterating through tests, we have a design that supports events.

Comment 3

[Benjamin Smedberg]

Where is the dividing line when you use the chrome-style harness? Are you running the chrome harness in the chrome process and each test in the content process? If so, how are you loading the chrome harness, since by default wouldn't it be in a Firefox/Fennec tab which might load it in the content process?

Perhaps we can break this down into a few separate problems:

* what process should the test harness run in?
* how does the test harness get privileges to do what it needs?
* how does each test communicate with the test harness?

It seems to me that the API between the test and the test harness is in general pretty small: it basically communicates a few messages like "test passed/failed" and "I'm done", right?

Apart from normal functions available to web content, what special privileges does the harness need? If we could just write code to expose those small set of APIs via the message manager (I'm pretty sure we can), that should be the simplest path here, and just run the harness and tests as content.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Here is the communication between a test and the harness (http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/):
* look for parentrunner in simpletest and it outlines, testFinished, Logger, and requestLongerTimeout()
* also in simpletest.js (http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#268), we use the focus manager to figure out who has the focus (this is repeated twice)
* log results (http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js), I have code which already does this from a regular html page
* Test runner sets url: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#139.
* test runner makes iframe: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/TestRunner.js#85
* shutdown on tests finished: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/quit.js 

Unfortunately my attempts at removing all of the privileged code has not resulted in simple tests running.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis (WIP)

an work in progress, but open for feedback on approach.  This was working great (I have hacks in the code for 127.0.0.1 instead of mochi.test) and then I updated my source code and rebuilt resulting in issues getting standalone fennec to load a webpage (throbber keeps spinning).  This is the same symptom I saw when using the env variable NECKO_E10S_HTTP=1 (hence the hack), but this was on all pages.  It appears that my scripts load, but 'onload' is never called (related to bug 562695?)  

Also this patch butchers a few things up that would not be 'backward compatible' with non e10s builds.  

This patch supports:
* file logging
* close-when-done

It doesn't support
* focus (harness does this a lot: http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/SimpleTest.js#256)
* getting the individual tests to run that require elevated privileges.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis (WIP 2.0)

updated patch with waitForFocus() code

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis 1.0

current patch, waiting for green try, then will ask for reviews.  There is some cleanup with dump statements I would like to remove as well.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 450428 [details] [diff] [review]
mochitest harness fennectrolysis 1.0

ted, I would appreciate some feedback on the SimpleTest.js::waitForFocus() approach.  Specifically if this is something we should do for all IPC/nonIPC stuff, or if we should strive to move everything into a separate file (ipcevents.js)

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis 2.0

updated patch to:
* remove all code paths when not mochitest plain.
* use file logging in content, no more proxying
* remove code paths (via check) for mochitest-plain, only use old method
* only do 1 check (try to set a pref) and use that value in all checks

I had two recurring issues while running the previous patch on try servers:
* TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_tree_hier.xul | Exited with code 6 during test run (actual process crash)
* TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/table/test_struct_ariatreegrid.html | Exited with code 6 during test run (actual process crash)

Both of these errors yielded this assertion:
Assertion failure: StackBase(cx->fp) + stackDepth <= cx->regs->sp, at /builds/slave/tryserver-linux-debug/build/js/src/jsinterp.cpp:122

By removing all the code paths and checking only once, I have seen better success on try server.  This patch is an updated version of the last try server run (try was down for the better part of the last day) which works on e10s.

One odd thing here is I am able to do file logging from content and was not able to do it from chrome-process-content via a messageManager.loadFrameScript() as I had been doing in the previous patch.

Asking for feedback on this patch to get a general idea if this approach is what we want checked in.  Once I am green on try server, I will ask for official review.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis 3.0

this patch is green on try server and runs a series of tests on f10s.  

This depends on bug 572149 to land, but otherwise we are ready to go (pending any review comments).

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 452227 [details] [diff] [review]
mochitest harness fennectrolysis 3.0

>+++ b/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
>@@ -1,15 +1,22 @@
> /**
>  * MozillaFileLogger, a log listener that can write to a local file.
>  */
>-try {
>-  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>-  const Cc = Components.classes;
>-  const Ci = Components.interfaces;
>+
>+  try {
>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+
>+    //NOTE: using const Cc aborts loading of script and is not caught by the try/catch
This sounds like a bug. Have you filed it?


>+function contentDispatchEvent(type, data, sync) {
>+  if (typeof(data) == "undefined") {
>+    data = {};
>+  }
>+  var element = document.createElement("myExtensionDataElement");
>+  element.setAttribute("sync", sync);
>+  element.setAttribute("type", type);
>+  element.setAttribute("data", JSON.stringify(data));
>+  document.documentElement.appendChild(element);
>+
>+  var evt = document.createEvent("Event");
>+  evt.initEvent("contentEvent", true, false);
>+
>+  element.dispatchEvent(evt);
>+}
This all is strange.
Couldn't you just create "datacontainerevent" and dispatch it to
window or document.
Something like 
var e = document.createEvent("datacontainerevent");
e.initEvent("contentEvent", true, false);
e.setData("sync", sync);
e.setData("type", type);
e.setData("data", JSON.stringify(data));
document.dispatchEvent(e);

And then when handling the event, use getData method.

Comment 12

[cmtalbert]

Comment on attachment 452227 [details] [diff] [review]
mochitest harness fennectrolysis 3.0

>diff --git a/testing/mochitest/ipc-overlay.xul b/testing/mochitest/ipc-overlay.xul
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/ipc-overlay.xul
>@@ -0,0 +1,38 @@
I think this needs a license header.

>diff --git a/testing/mochitest/ipc.js b/testing/mochitest/ipc.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/ipc.js
This also needs a license header.

>@@ -0,0 +1,164 @@
>+function ipcEvent(e) {
>+    var sync = e.target.getAttribute("sync");
>+    var type = e.target.getAttribute("type");
>+    var data = JSON.parse(e.target.getAttribute("data"));
>+
>+    switch(type) {
>+    case 'LoggerInit':
>+      MozillaFileLogger.init(data.filename);
>+      break;
>+    case 'Logger':
>+      var logger = MozillaFileLogger.getLogCallback();
>+      logger({"num":data.num, "level":data.level, "info": Array(data.info)});
>+      break;
>+    case 'LoggerClose':
>+      MozillaFileLogger.close();
>+      break;
>+    case 'waitForFocus':
>+      if (content)
>+        var wrapper = content.wrappedJSObject.frames[0].SimpleTest;
>+      else
>+        var wrapper = SimpleTest;
>+      ipctest.waitForFocus(wrapper[data.callback], data.targetWindow, data.expectBlankPage);
>+      break;
>+    default:
>+      if (sync == 1) {
>+        return sendSyncMessage("chromeEvent", {"type":type, "data":data});
>+      } else {
>+        sendAsyncMessage("chromeEvent", {"type":type, "data":data});
>+      }
>+    }
>+};
>+addEventListener("contentEvent", function (e) { ipcEvent(e); }, false, true);
We should remove this event listener when we stop running tests otherwise, it might get flagged as a leak.

>+
>+var ipctest = {};
>+
>+ipctest.waitForFocus_started = false;
>+ipctest.waitForFocus_loaded = false;
>+ipctest.waitForFocus_focused = false;
>+
>+//TODO: need to test the targetWindow parameter
Follow on bug with a test case?

>+ipctest.waitForFocus = function (callback, targetWindow, expectBlankPage) {
>+
>+    if (targetWindow && targetWindow != undefined) {
>+      var tempID = targetWindow;
>+      targetWindow = null;
>+      var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                         .getService(Components.interfaces.nsIWindowMediator);
>+      var wm_enum = wm.getXULWindowEnumerator(null);
>+
>+      while(wm_enum.hasMoreElements()) {
>+        var win = wm_enum.getNext().QueryInterface(Components.interfaces.nsIXULWindow)
>+                  .docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindow)
>+                  .content.wrappedJSObject;
>+
>+        var domutils = win.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
>+                           getInterface(Components.interfaces.nsIDOMWindowUtils);
>+
You're switching here between Cc & Ci versus Components.classes and Components.interfaces paradigms.  If Cc and Ci are available, use them throughout.

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>--- a/testing/mochitest/runtests.py.in
>+++ b/testing/mochitest/runtests.py.in
>     if options.browserChrome:
>       manifestFile.write("""overlay chrome://navigator/content/navigator.xul chrome://mochikit/content/browser-test-overlay.xul
> overlay chrome://browser/content/browser.xul chrome://mochikit/content/browser-test-overlay.xul
> """)
>+    elif ((options.chrome == False) and (options.a11y == False)):
>+      manifestFile.write("overlay chrome://browser/content/browser.xul chrome://mochikit/content/ipc-overlay.xul")
>+      
So, in other words, only use ipc-overlay in plain mochitests.  Can we get a comment that states that so people not so familiar with the flavors of mochitest can grok this code.  (It will be important the next time someone adds a new flavor).

>diff --git a/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js b/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
>--- a/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
>+++ b/testing/mochitest/tests/SimpleTest/MozillaFileLogger.js
>@@ -1,15 +1,22 @@
> /**
>  * MozillaFileLogger, a log listener that can write to a local file.
>  */
>-try {
>-  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>-  const Cc = Components.classes;
>-  const Ci = Components.interfaces;
>+
>+  try {
>+    netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+
>+    //NOTE: using const Cc aborts loading of script and is not caught by the try/catch
>+    if (!(Components === undefined) && Cc === undefined) {
>+      var Cc = Components.classes;
>+      var Ci = Components.interfaces;
>+    }
>+  } catch (ex) {} //running in ipcMode-chrome
>+
Why not do an if (Cc === undefined) { const Cc = Components.classes; } etc?


>   const FOSTREAM_CID = "@mozilla.org/network/file-output-stream;1";
>   const LF_CID = "@mozilla.org/file/local;1";
>   
>   // File status flags. It is a bitwise OR of the following bit flags.
>   // Only one of the first three flags below may be used.
>   const PR_READ_ONLY    = 0x01; // Open for reading only.
>   const PR_WRITE_ONLY   = 0x02; // Open for writing only.
>   const PR_READ_WRITE   = 0x04; // Open for reading and writing.
>@@ -26,42 +33,70 @@ try {
>   
>   // If set, each write will wait for both the file data
>   // and file status to be physically updated.
>   const PR_SYNC         = 0x40;
>   
>   // If the file does not exist, the file is created. If the file already
>   // exists, no action and NULL is returned.
>   const PR_EXCL         = 0x80;
>-} catch (ex) {
>-  // probably not running in the test harness
>-}
Why do you change what is covered in this try/catch?  I think the scope of the try/catch should stay the same and if you need it, you should add a second one within the larger existing one that covers all the file constants rahter than changing the scope of the try/catch.

>+    try {
>+      netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
>+    } catch(ex) {} //running in ipcMode-chrome
I realize that we are in the midst of logging a message, so if we're throwing here we likely can't write to a file, but the number of these try { ..stuff..} catch(ex){} patterns is alarming.  We could fail in here and never know why.  I'd considering "dump"ing an error message in the catch just to note the error and avoiding silent failures later.  I guess it might not be needed because the only way you'd throw is by running in IPC mode and if you're in IPC mode you're not executing this code.  Hmm...well, whichever you prefer.

>diff --git a/testing/mochitest/tests/SimpleTest/SimpleTest.js b/testing/mochitest/tests/SimpleTest/SimpleTest.js
>--- a/testing/mochitest/tests/SimpleTest/SimpleTest.js
>+++ b/testing/mochitest/tests/SimpleTest/SimpleTest.js
>@@ -21,16 +21,22 @@ if (typeof(SimpleTest) == "undefined") {
> var parentRunner = null;
> if (typeof(parent) != "undefined" && parent.TestRunner) {
>     parentRunner = parent.TestRunner;
> } else if (parent && parent.wrappedJSObject &&
>            parent.wrappedJSObject.TestRunner) {
>     parentRunner = parent.wrappedJSObject.TestRunner;
> }
> 
>+//Simple test to see if we are running in e10s IPC
>+var ipcMode = false;
>+if (parentRunner) {
>+  ipcMode = parentRunner.ipcMode;
>+}
>+
> // Check to see if the TestRunner is present and has logging
> if (parentRunner) {
>     SimpleTest._logEnabled = parentRunner.logEnabled;
> }
> 
> SimpleTest._tests = [];
> SimpleTest._stopOnLoad = true;
> 
>@@ -249,16 +255,39 @@ SimpleTest.waitForFocus_focused = false;
>  * @param callback
>  *        function called when load and focus are complete
>  * @param targetWindow
>  *        optional window to be loaded and focused, defaults to 'window'
>  * @param expectBlankPage
>  *        true if targetWindow.location is 'about:blank'. Defaults to false
>  */
> SimpleTest.waitForFocus = function (callback, targetWindow, expectBlankPage) {
>+
>+      if (parent && parent.ipcWaitForFocus != undefined) {
>+        parent.contentAsyncEvent("waitForFocus", jsonData);
>+      } else {
>+        parent.contentAsyncEvent("waitForFocus", jsonData);
>+      }
This if statement is unnecessary (you call the same thing if either the condition is true or not).  So, either that's a bug, or its completely unneeded.

> SimpleTest.waitForClipboard = function(aExpectedVal, aSetupFn, aSuccessFn, aFailureFn) {
>+    if (ipcMode) {
>+      //TODO: support waitForClipboard via events to chrome
>+      return;
>+    }
File a follow up bug on this please and note the bug number here.  I guess since this runs green on try, we don't have any tests using this function in IPC mode.  If we did, then I'd like to see us write a message out to the logfile to help with debugging the test failure in this case.

>diff --git a/testing/mochitest/tests/SimpleTest/TestRunner.js b/testing/mochitest/tests/SimpleTest/TestRunner.js
>--- a/testing/mochitest/tests/SimpleTest/TestRunner.js
>+++ b/testing/mochitest/tests/SimpleTest/TestRunner.js
>@@ -1,8 +1,40 @@
>+// e10s event dispatcher from content->chrome
>+
>+/*
>+ * type = eventName (QuitApplication, LoggerInit, LoggerClose, Logger, GetPref, SetPref)
>+ * data = json object {"filename":filename} <- for LoggerInit
>+ */
Why not join these two comments in one /* */ style comment?

>+function contentSyncEvent(type, data) {
>+  contentDispatchEvent(type, data, 1);
>+}
>+
>+function contentAsyncEvent(type, data) {
>+  contentDispatchEvent(type, data, 0);
>+}
>+
>+
>+
Three blank lines seems a little excessive ^

r=me with those things addressed.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis 3.1

carry over review from ctalbert.  updated to fix leak where we add event listener but never send the event to trigger the removal (non IPC case).

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mochitest harness fennectrolysis 3.14

oops, previous attachment had somehow removed the license header from reftest.xul?  Not sure why that happened...fixed.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

landed on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/750d05734c1d