582472 - Add a SpecialPowers object that replaces UniversalXPConnect usage in Mochitest

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Robert Sayre]

APIs can get added to the Special Powers extension as needed.

Comment 1

[Robert Sayre]

Attachment: an extension that adds a window.SpecialPowers object

Comment 2

[Robert Sayre]

Attachment: an extension that adds a window.SpecialPowers object

missed a test

Comment 3

[(not currently active) Ted Mielczarek]

I thought we had a bug on this, but I think it got duped to bug 549539.

Also, is this going to work with e10s, or is that a separate issue? Joel has been doing some work making Mochitest work with e10s, and some of it involves moving the enablePrivilege bits out of tests into the harness.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 460748 [details] [diff] [review]
an extension that adds a window.SpecialPowers object

>diff --git a/testing/mochitest/runtests.py.in b/testing/mochitest/runtests.py.in
>+  def installSpecialPowersExtension(self, browserEnv, options):
>+    """ install the Special Powers extension for special testing capabilities """
>+    extensionSource = self.getFullPath("_tests/testing/mochitest/specialpowers")
>+
>+    self.automation.log.info("INFO | runtests.py | Installing extension at " + extensionSource +
>+                             " to " + options.profilePath + ".")
>+    
>+    self.automation.installExtension(extensionSource, options.profilePath, "[e-mail]")
>+
>+    # now run once to register
>+    self.automation.runApp(None, browserEnv, options.app, options.profilePath,
>+                                 ['-silent'],
>+                                 utilityPath = options.utilityPath,
>+                                 xrePath=options.xrePath,
>+                                 symbolsPath=options.symbolsPath)

This shouldn't be necessary anymore since bug 576553 landed.

Comment 5

[Robert Sayre]

Attachment: an extension that adds a window.SpecialPowers object

ted is right, no need to run the app silently

Comment 6

[cmtalbert]

Comment on attachment 460949 [details] [diff] [review]
an extension that adds a window.SpecialPowers object

>diff --git a/testing/mochitest/specialpowers/Makefile.in b/testing/mochitest/specialpowers/Makefile.in
>+DEPTH		= ../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
Nit: stray tab in there ^

>diff --git a/testing/mochitest/specialpowers/components/Makefile.in b/testing/mochitest/specialpowers/components
>+DEPTH		= ../../../..
>+topsrcdir	= @top_srcdir@
>+srcdir		= @srcdir@
Nit: same tab ^

>diff --git a/testing/mochitest/specialpowers/components/SpecialPowers.js b/testing/mochitest/specialpowers/components/SpecialPowers.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/specialpowers/components/SpecialPowers.js
>+/*
>+ * 
>+ */
Nit: Did you intend to put text there ^?  Remove the empty comment if not.
>+
>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+// Based on:
>+// https://bugzilla.mozilla.org/show_bug.cgi?id=549539
>+// https://bug549539.bugzilla.mozilla.org/attachment.cgi?id=429661
>+// https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_1.9.3
>+// http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#3240
>+// https://developer.mozilla.org/en/how_to_build_an_xpcom_component_in_javascript
>+
Nit: This could go in the empty comment.

>+  observe: function(aSubject, aTopic, aData)
>+  {
>+    if (aTopic == "profile-after-change") {
>+      this.init();
>+    } else if (aTopic == "content-document-global-created") {
I'm not finding this topic in mxr anywhere.  So I can't really say why you'd sometimes get something back that is not a wrappedJSObject.

>+      var w = aSubject.wrappedJSObject;
>+
>+      if (w) {      
>+        w.SpecialPowers = SpecialPowers;
>+      } else {
>+        // I don't understand why this happens.  Some chrome windows sneak in here?
This is going to be a real pain to debug if it fails silently.  Theoretically though, if the topic isn't a wrappedJSObject, the likelihood of it executing a script that needs privs is close to 0 (as I understand things).  However, I'd be more comfortable if there were some way we can log this error case. That said, I don't think we have access to the mochitest logging system from inside this extension.  If we threw an exception here, would that be enough to alert the test that something went wrong?

>diff --git a/testing/mochitest/specialpowers/install.rdf b/testing/mochitest/specialpowers/install.rdf
>+    <!-- Target Application this extension can install into, 
>+         with minimum and maximum supported versions. -->
>+    <em:targetApplication>
>+      <Description>
>+	<em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
>+        <em:minVersion>4.0b2pre</em:minVersion>
>+        <em:maxVersion>9</em:maxVersion>
I'd rather you make the target application [e-mail], then it will work seamlessly in both fennec and firefox.  Reftest does this and doesn't hard code the version numbers: http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/install.rdf#10.  I figure we ought to follow that lead and handle the version numbers the same way.

That's it.  My major concern here is the silent failure in the case that aSubject does not have a wrappedJSObject.  If we can find someway not to silently fail in that case, I'm happy.  This looks good r=ctalbert with those changes.

Comment 7

[Justin Lebar (not reading bugmail)]

jst asked if I could push this through, so I'm going to try.

Comment 8

[Justin Lebar (not reading bugmail)]

(In reply to comment #6)
> Comment on attachment 460949 [details] [diff] [review]
> an extension that adds a window.SpecialPowers object
> 
> >diff --git a/testing/mochitest/specialpowers/Makefile.in b/testing/mochitest/specialpowers/Makefile.in
> >+DEPTH		= ../../..
> >+topsrcdir	= @top_srcdir@
> >+srcdir		= @srcdir@
> Nit: stray tab in there ^
> 
> >diff --git a/testing/mochitest/specialpowers/components/Makefile.in b/testing/mochitest/specialpowers/components
> >+DEPTH		= ../../../..
> >+topsrcdir	= @top_srcdir@
> >+srcdir		= @srcdir@
> Nit: same tab ^

Not to distract ourselves with whitespace, but those tabs render fine in the original file; it's just when you put "> >+" in front of them that they don't line up.

Comment 9

[(not currently active) Ted Mielczarek]

Our current conventions discourage tabs in Makefiles except where explicitly required (in the commands of rules).

Comment 10

[Justin Lebar (not reading bugmail)]

(In reply to comment #9)
> Our current conventions discourage tabs in Makefiles except where explicitly
> required (in the commands of rules).

Okay then; those tabs are done for!

(In reply to comment #6)
> >+  observe: function(aSubject, aTopic, aData)
> >+  {
> >+    if (aTopic == "profile-after-change") {
> >+      this.init();
> >+    } else if (aTopic == "content-document-global-created") {
> I'm not finding this topic in mxr anywhere.  So I can't really say why you'd
> sometimes get something back that is not a wrappedJSObject.

Is it http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2075 ?

But I still don't see why you wouldn't get a wrappedJSObject.  Jonas, do you have any idea?

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does the wrappedJSObject issue happen randomly? Or consistently on a few tests? Can you alert the .document.documentURL of the object that doesn't have a wrappedJSObject?

The notification is sent out for all documents that doesn't have the system-principal as the NodePrincipal. However we determine if we should wrap based on if it's a nsGlobalChromeWindow or not.

So if we ever create a nsGlobalChromeWindow that doesn't have system principals, then the content-document-global-created notification will fire with an object without a .wrappedJSObject.

However that seems really scary if it happens. Would love to pin down when that is.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

ok, bz says that this can happen under quite non-scary (but "sad") circumstances. Such as chrome containing <iframe src="http://example.com">.

In the interest of getting this show on the road, I say lets start with keeping this patch as is. Blake says that the wrapper code is going to change to looking at the principal, which means that all objects will be getting a .wrappedJSObject.

I'm not really worried about "silent fails". It should be quite obvious in the actual test if it tries to access the .SpecialPowers object and it's not there.

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Addresses most review comments

This addresses the cosmetic review comments.  I couldn't get the suggested changes to install.rdf to work; I think those #expand lines weren't being blessed by the appropriate build magic when I added them to this patch.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Addresses most review comments v2

Whoops -- forgot to qref.

Comment 15

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

I am curious how this will with in IPC.  My understanding is that this will not have access to both chrome and content code?  If this lives in just the chrome or content process, we would need to access this via the messageManager which would make most transactions async.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

First off, I think we should worry about this when we get there. In fact, moving to using a SpecialPowers will probably make transition easier because we'll "just" have to modify its code most of the time.

But I think the SpecialPowers object can run in the content process. My understanding is that even the content process will be able to run privileged code. In the cases when we need information from the chrome process we should be able to make blocking IPC calls to the chrome process.

Comment 17

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

For all Fennec automation we are using IPC, so we are already there (for the most part.)

The IPC message manager does have a sendSyncMessage (https://wiki.mozilla.org/Content_Process_Event_Handlers) option, but that is assuming you are actually inside the messageManager context.  For a content process to access chrome (such as window.focus(), quit, logging, setpref, etc...) we need to dispatch an event that the messageManager can pick up.  This part is 100% async.  It would be cool if specialPowers could somehow bridge that gap and provide a synchronous form of communication from content->messageManager/chrome.

Another problem we had with getting tests running with IPC turned on was that for mochitest-chrome (which shares the same focus code as plain) we were unable to resolve focus for all window.open cases because the messageManager listener code was not running on the new windows.  

For reference here is the patch that is checked in to get mochitests (plain only) working with IPC:
https://bug567417.bugzilla.mozilla.org/attachment.cgi?id=453832

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm a bit confused about the problem you're trying to solve.

Note that we're just attempting to replace the code which gets UniversalXPConnect rights and uses various XPCOM components using that.

This code is *already* living in the content process. So clearly all those XPCOM components are already available to the content process.

All this bug is about is moving that code to a javascript object running with chrome privileges. No code is intended to be moved to another process.

Anything that's working in a electrolysis world right now, should continue to do so with this new setup. Anything that's not working today I'd prefer to have separate bugs filed on so we can talk about specifics there.

We really need to get this component landed ASAP so that we can start with the huge undertaking of rewriting hundreds of tests to use this. That work is currently blocked on this bug.

I'm not really interested in further blocking that work while figuring out how we'll do various things in an electrolysis world. Lets do that in parallel in another bug.

Or am I misunderstanding something?

Comment 19

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

The only concern I have is if we edit a few hundred test cases (~450), then we might have to edit them again in a couple months when we are getting them ready for e10s(IPC).

My understanding is that both specialPowers and IPC end up with the same side effect of removing all elevated privileged code from our mochitest-plain tests.  Doing the work once would be ideal, minimizing the duplication of work should be possible.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree that we'll want to avoid rewriting. But I suspect that as long as we at some point in the future are able to make synchronous calls from the content process to the chrome process, then we can maintain almost any API that we set up now.

Though if you have concrete ideas for how we can rewrite the component as to allow easier electrolysis integration in the future, then I'm all ears. Note that we can always make individual APIs exposed by the component be asynchronous. This bug doesn't really implement any APIs.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can we do this:

* Land this patch as is, since it only adds the needed infrastructure and
  none of the actual APIs.
* As we start adding APIs and moving tests to them (will happen in separate
  bugs), if there are APIs that we know doesn't work in e10s land without
  calling into the chrome process, make the API asynchronous.
* When making mochitest run under electrolysis, implement those asynchronous
  APIs by calling into the chrome process.

I would also think it's ok to make the API synchronous, and just deal with this by adding the capability of making synchronous calls into chrome. But if people think that will be harder then I'm fine with async too.

Let me know what you think.

Comment 22

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

sicking: I am not sure how we would make synchronous calls from raw content into chrome, that is my big hang up here.

smaug: do you know of a way (or in the future) that we can do synchronous calls from content->chrome (comment 17)?

Comment 23

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

I don't really understand comment 17. Dispatching DOM events is synchronous, and
the events are synchronously handled in TabChildGlobal context and
the event listeners can use sendSyncMessage.

Also, we have now process message manager, so that child process
can send messages also using
var cmm = Components.classes["@mozilla.org/childprocessmessagemanager;1"]
            .getService(Components.interfaces.nsISyncMessageSender);
cmm.sendSyncMessage("messageName", "jsondata");

In chrome process one needs to add listener to
Components.classes["@mozilla.org/parentprocessmessagemanager;1"]
  .getService(Components.interfaces.nsIFrameMessageManager)

Comment 24

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

So here is an example we should keep in mind:

http://mxr.mozilla.org/mozilla-central/source/gfx/tests/mochitest/test_bug513439.html?force=1#27
var oldVal = layoutCSSBranch.getCharPref("devPixelsPerPx");

To do this in IPC, we would need to do something like this:
//setup steps
dispatchEvent('getCharPref', 'devPixelsPerPx', callbackName);

function callbackName() {
 //do the test
}
//cleanup


The difference here is we cannot make a call to getCharPref() and return the value as if it were a function.  We have to wait for messageManager to dispatch an event to the original contentProcess code.  

This is where my concern is with special powers.  If we do a:
var oldVal = specialPowers.getCharPref("devPixelsPerPx");

that would be an easy fix to the test cases.  But if this doesn't work in IPC, then we are back to the drawing board and editing all the tests again.


Is there something I am overlooking here?

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2.1

Removing some |dump|s which made it into the last patch.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, r-=me unless we rename the object SuperPowers

(ok, i'm mostly kidding)

Comment 27

[cmtalbert]

Attachment: This version should work with IPC enabled

This version of the patch should provide all the necessary tunneling to make the patch work in IPC.  I went ahead and created a setpref/getpref API just for verification that the system was working.

I think we may want to use contentPrefs there in place of that API, but at least we can start to get a sense of what an API for this will look like.

Comment 28

[cmtalbert]

Attachment: A listing of all the places we use EnablePrivilege

This is an accounting of all the places we use enable privilege and attempts to detect why we use enable privilege in that case.  I'll attach the script that generates this file.  As you can see many of these are simply used to get direct access to some service, so we may need a generic way to do this.  Cjones was thinking we might be able to use cross-process object wrappers for this.

Comment 29

[cmtalbert]

Attachment: The script used to find uses of enable privilege

This is the rough little script that finds uses of enable privilege.  The regex's it uses are pretty simple and could probably be tweaked, but to get a rough accounting, it does ok.

To use:
1. Copy to your source tree directory
2. python findEP.py
3. Go get coffee (takes about 3 minutes on my mac laptop), I also didn't spend any effort making it performant.

Comment 30

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

Comment on attachment 474847 [details] [diff] [review]
This version should work with IPC enabled


>+function SpecialPowers() {}
>+
>+var SpecialPowers = {
>+  sanityCheck: function() { return "foo"; },
>+  // Here is an example of a synchronous chrome call
>+  getPref: function(aPrefName, aPrefType, aIid) {
>+    var msg = {};
>+    if (aIid) {
>+      // We overload value to handle complex prefs TODO: fix
>+      msg = {'op':'get', 'prefName': aPrefName, 'prefType':aPrefType, 'prefValue':[aIid]};
>+    } else {
>+      msg = {'op':'get', 'prefName': aPrefName,'prefType': aPrefType};
>+    }
>+    return(sendSyncMessage('prefService', msg)[0]);
For all the messages it might be better to use some prefix in the name.
Perhaps SPPrefService etc.?


>+// Attach our API to the window
>+try {
>+  var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"].
>+                             getService(Components.interfaces.nsIWindowMediator);
>+  var win = wm.getMostRecentWindow(null);
>+  dump("\n\n DBG: this is win.content - " + win.content + "\n\n");
>+  
>+  var contentwin = win.content.wrappedJSObject;
>+  if (contentwin) {
>+    contentwin.SpecialPowers = SpecialPowers;
>+  } else {
>+    dump("TEST-INFO | specialpowers.js | Cannot attach special powers to window: " + win + "\n");
>+  }
>+} catch(ex) {
>+  dump("\n\nDBG: Failed to attach specialpowers to window exception: " + ex + "\n\n");
>+}
I don't understand this. Why you use windowmediator? Couldn't you just use "content".
And for subframes you probably need to set SpecialPowers when the window is created (using the
something similar to InstalTrigger)

Though, I'm not sure who runs specialpowers.js.

Comment 31

[cmtalbert]

Attachment: with ipc version 2

(In reply to comment #30)
> For all the messages it might be better to use some prefix in the name.
> Perhaps SPPrefService etc.?
Good idea.  Prefixing "SP".

> I don't understand this. Why you use windowmediator? Couldn't you just use
> "content".
> And for subframes you probably need to set SpecialPowers when the window is
> created (using the
> something similar to InstalTrigger)
Originally I was using content, but it stopped working when I started changing to a frame script.  The windowmediator trick seemed to work, but I went back and simply used "content" and it worked fine.  So I changed it to work with content.  The observers in SpecialPowersObserver ensure that this gets attached to all subframes.  I included a test in this patch that ensures it is loaded on subframes. (You have to run the test specifically by name for it to report, due to the iframe being its own universe.  Here's the command line:
cd <objdir>/_tests/testing/mochitest
python runtests.py --test-path=Harness_sanity/test_SpecialPowersExtension2.html --autorun

> Though, I'm not sure who runs specialpowers.js.
It is attached to every content/frame/window object created by the system via the code in specialpowersobserver.js, but it looks like I forgot to hg add it in the earlier patch. Sorry about that.

Comment 32

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

Comment on attachment 478487 [details] [diff] [review]
with ipc version 2

Someone familiar with js component handling and addons should probably
review this too.

We probably don't want the dump()s in the final patch, at least the
ones which aren't use inside catch() {}

Comment 33

[cmtalbert]

Attachment: Final Version

This is yet another version of special powers.  I wanted to test it with fennec to make sure that we're all correct and that it worked inside of a true IPC mode application.  It actually didn't.  I found that I had to also have an observer for the content-document-global-created notification inside the content process as well (loaded in the frame script).  So, I had to add that.  Unfortunately, that has an odd behavior where if the frame script observer gets unregistered, we lose all visibility into the content process.  Since the content process is already spawned, our main chrome-based observer will *not* see further events from the child and therefore we can't be re-spawned.  Dougt, does this behavior make sense to you or does this seem like a bug?

I worry that doing it this way will leak.  On Desktop fennec, we're leaking all over the place already, but among those leaks is a nsObserverService, so I worry that might be this patch's doing.  However, on Firefox we don't leak (!?) in spite of register being called far more times?  Those numbers are fairly skeptical, I'm going to test a bit more on both and run it through try server.

Also, in this patch in order to make it work for Fennec, I had to use the toolkit@mozilla ID and the Gecko version, and in order to do that properly I had to do some heavy editing of the makefiles and add a jar.mn script.  So, Ted, that part of the review is for you :)

Comment 34

[cmtalbert]

Attachment: SpecialPowers 2.1 - correction to mochitest

Comment 35

[cmtalbert]

Reviewed the logs, and ensured that everything still worked after compartments.  There are 8 bytes fewer leaked on Fennec when run with Special Powers rather than run without it.  So, that seems like an amount nearly too small to measure.  No special powers objects (nsIObserver's,etc) are leaked by including the extension, so I think it's ok from that perspective, but I sure wish someone would explain to me what's cleaning up the listeners that are not being unregistered.  I'm running again on try server right now (a run before compartments landed was green), and I'll report on those results.

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The observerservice should be releasing all references to all observers during XPCOM shutdown when the observerservice itself is shut down. So you only need to unregister observers if you want them to go away before that.

Comment 37

[(not currently active) Ted Mielczarek]

Comment on attachment 483493 [details] [diff] [review]
SpecialPowers 2.1 - correction to mochitest

>diff --git a/testing/mochitest/Makefile.in b/testing/mochitest/Makefile.in
>--- a/testing/mochitest/Makefile.in
>+++ b/testing/mochitest/Makefile.in
>@@ -44,16 +44,17 @@ relativesrcdir  = testing/mochitest
> include $(DEPTH)/config/autoconf.mk
> 
> DIRS =	MochiKit \
> 	static \
> 	dynamic \
> 	tests \
> 	chrome \
> 	ssltunnel \
>+  specialpowers \
> 	$(NULL)

Can you just edit the rest of these to be consistent (two-space indent) while you're here?

>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
>@@ -454,16 +454,24 @@ class Mochitest(object):
>     self.server.stop()
> 
>   def getLogFilePath(self, logFile):
>     """ return the log file path relative to the device we are testing on, in most cases 
>         it will be the full path on the local system
>     """
>     return self.getFullPath(logFile)
> 
>+  def installSpecialPowersExtension(self, browserEnv, options):
>+    """ install the Special Powers extension for special testing capabilities """
>+    extensionSource = os.path.normpath(os.path.join(self.SCRIPT_DIRECTORY, "specialpowers"))
>+    self.automation.log.info("INFO | runtests.py | Installing extension at " + extensionSource +
>+                             " to " + options.profilePath + ".")

I prefer string interpolation with % to concatenation with + when you're joining more than 2 strings.

>+    self.automation.installExtension(extensionSource, options.profilePath, "[e-mail]")
>+    self.automation.log.info("INFO | runtests.py | Done installing extension.")
>+
>diff --git a/testing/mochitest/specialpowers/Makefile.in b/testing/mochitest/specialpowers/Makefile.in
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/specialpowers/Makefile.in
>+DIST_FILES = install.rdf \
>+						 chrome.manifest \
>+						 $(NULL)
>+

I don't kow what happened here, but your indentation is all screwed up. Also, if you're doing a multi-line variable set, I prefer that it looks like:
DIST_FILES = \
  install.rdf \
  chrome.manifest \
  $(NULL)

>diff --git a/testing/mochitest/specialpowers/components/SpecialPowersObserver.js b/testing/mochitest/specialpowers/components/SpecialPowersObserver.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/specialpowers/components/SpecialPowersObserver.js
>+
>+  init: function()
>+  {
>+    var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);

If you import Services.jsm, you can just use Services.obs instead of having to getService it all over:
https://developer.mozilla.org/en/JavaScript_code_modules/Services.jsm


>diff --git a/testing/mochitest/specialpowers/content/specialpowers.js b/testing/mochitest/specialpowers/content/specialpowers.js
>new file mode 100644
>--- /dev/null
>+++ b/testing/mochitest/specialpowers/content/specialpowers.js
>+
>+var SpecialPowers = {
>+  sanityCheck: function() { return "foo"; },
>+  // Here is an example of a synchronous chrome call
>+  getPref: function(aPrefName, aPrefType, aIid) {

I feel like it might make sense to mimic the nsIPrefService API a little more closely here, and provide getBoolPref / setBoolPref etc, to make it easy to change the huge amount of tests we're going to have to change. If we provide essentially the same API, it should be a much simpler change. They should be pretty simple wrappers around what you've already implemented here.

>diff --git a/testing/mochitest/tests/Makefile.in b/testing/mochitest/tests/Makefile.in
>--- a/testing/mochitest/tests/Makefile.in
>+++ b/testing/mochitest/tests/Makefile.in
>@@ -49,14 +49,17 @@ PARALLEL_DIRS = \
> 	SimpleTest \
> 	browser \
> 	$(NULL)
> 
> include $(topsrcdir)/config/rules.mk
> 
> _TEST_FILES = \
> 	test_sanity.html \
>+	test_SpecialPowersExtension.html \
>+	test_SpecialPowersExtension2.html \
>+	file_SpecialPowersFrame1.html \
> 	$(NULL)

Just fix the indentation here to be two-space for everything in this block.

The sanity-tests are nice!

I don't really know anything about MessageManager, so I can't comment on those bits of the patch, but the rest looks fine to me with those few comments addressed.

Comment 38

[cmtalbert]

Jonas, thanks for explaining that, I didn't realize I didn't need to unregister the observers, so that explains why it isn't leaking.  I'll update the code to take out the commented out unregister code.

Thanks for the review Ted.  I'm going to make those changes, and I'm going to add one more thing from my try server run -- in the current patch, specialpowers is installed in *all* mochitest flavors, even chrome, browser-chrome and a11y which all run from chrome space.  Since that's silly, I'm going to gate the installation of the special powers extension so that it is only installed on mochitest-plain.

An interesting bit of other news from the try server runs: It looks like the double-observer trick to get things to work in Fennec is causing havoc with a bunch of our random orange tests.  If you think about it, we're affecting the timing of every "load" event in the system to ensure that content has a specialPowers object on it.  That small change seems to force our intermittent tests to fail.  Unfortunately, it hasn't made them reproducible or predictable - my try server runs are now showing a random assortment of our intermittent tests timing out.  Not installing specialpowers on browserChrome and Chrome will likely help this to a degree.

Comment 39

[cmtalbert]

Attachment: Addresses review comments

This addresses Ted's review comments, removes a dump statement that wasn't in a catch block in the frame script, and adds in some protection against trying to use a content variable that is null or undefined inside the frame script.
Submitted to try as well.

Comment 40

[cmtalbert]

I've got it green on try, so cross your fingers and toes, it's on m-c as http://hg.mozilla.org/mozilla-central/rev/32f70e593bf6.

Comment 41

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I, for one, welcome our new SpecialPowers overlord.

Comment 42

[cmtalbert]

Well, it almost made it.

Backed out due to an intermittent failure on the special powers sanity test itself: http://hg.mozilla.org/mozilla-central/rev/bb05f8488944

Not at all sure what is up with that as this has never ever failed in any of my testing.

Comment 43

[cmtalbert]

(In reply to comment #41)
> I, for one, welcome our new SpecialPowers overlord.

Jonas, the only way I can see for the sanity test to fail the way it did would be for the special powers object to either:
* never capture the content document created event OR 
* be called on windows where the content object is always null. OR
* be called on window where the content object is not a wrappedJSObject

Are any of those situations prone to happen intermittently?  I'm bringing up an XP vm to try this out in in case it is something odd with our interactions on the XP APIs (we're still only running tests on windows 2003 server machines, which I think has the same API as XP).

Comment 44

[Josh Matthews [:jdm]]

For the record, I had to change the chrome manifest content line to read:

content specialpowers jar:chrome/specialpowers.jar!/content/

to make it work properly with a websocket test in a content process.

Comment 45

[Josh Matthews [:jdm]]

Furthermore, this seems to have recently been broken by bug 608669 when trying to do IPC tests.  With the patch from bug 608669 applied, we no longer get content-global-document-created observer notifications in the chrome process, so the SpecialPowersObserver never ends up setting up the content SpecialPowers object.  Without bug 608669 applied, we do get those notifications.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Sounds like perhaps this code should listen for chrome-document-global-created as well... though I wonder why chrome document globals are trying to use SpecialPowers at all.

Comment 47

[Josh Matthews [:jdm]]

Further furthermore, SpecialPowers.SetIntPref('foo', 0) and SpecialPowers.SetBoolPref('foo', false) fail because of these incorrect checks:

>var prefValue = aMessage.json.prefValue ? aMessage.json.prefValue : null;
...
>if (!prefName || !prefType  || !prefValue)
>  throw new SpecialPowersException("Invalid parameters for set in SPPrefService");

I changed it locally to be

>var prefValue = "prefValue" in aMessage.json.prefValue ? aMessage.json.prefValue : null;
...
>if (!prefName || !prefType || prefValue === null)

and it works just fine now.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

aMessage.json.prefValue != null is a simpler check that should be equivalent. Are there followup bugs for these issues?

Comment 49

[Josh Matthews [:jdm]]

I haven't filed any bugs yet, since the original hasn't landed.

Comment 50

[cmtalbert]

Thanks Josh for the pointers.  I ran the patch for almost two weeks in a record & replay vm trying to capture the error I saw on m-c when I checked it in (where the content-global-document-created events intermittently failed to fire on windows). I was never able to repro it.

In response to Gavin in comment 46, I think the chrome-side observer needs to watch for chrome-global-document-created in order to ensure the special powers frame script gets called when in IPC mode. I'll verify that on a fennec build after I make the changes.

Once I get these changes working here, I'll submit to try again and we'll see if we can get this damn thing finally landed.

Comment 51

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I landed some fixes to these notifications over in bug 608669. That might have fixed the problem you were seeing. But that's as much hoping as actually believing so :)

Comment 52

[cmtalbert]

Attachment: Updated patch with Josh's changes

I made the changes to have the chrome observer watch for both chrome and content global-document-created events so that it works both in IPC mode and without (verified on fennec desktop).  I also made the pref API changes.  I couldn't for the life of me figure out why my build builds with a flat extension while Josh's built with a jar'd extension.  I just forced the makefile to build the extension flat.  So hopefully it will fix the issue he saw with the chrome manifest.

My reasoning for keeping it flat is that it will be a lot nicer to simply change specialpowers.js directly when adding new capabilities rather than having to rebuild each time you make a change while trying to add a new specialpowers API.  It's sort of more in-line with the process for developing a mochitest if it is a flat extension.

And I'm submitting this version to try.  Keep your fingers crossed. :)

Comment 53

[cmtalbert]

And it exploded on try due to the patch slowing down mochitest too much (buildbot timeouts, test suite timeouts etc).  I think if I can gate on not watching chrome-g-d-c events in non-IPC mode (since they are unneeded) it will be better.  Submitting patch to do that to try and see if we are better off.

http://hg.mozilla.org/try/rev/e4ff5b7fb161

Comment 54

[cmtalbert]

Attachment: Version 4.9

This is the latest patch, there have been a few substantial changes now, so maybe it would be good to once-over it one more time.  I've made some changes while battling oranges caused by the introduction of the patch, but this version is looking good on try right now.  Here are the changes:

= runtests.py =
* moved the call to install the special powers inside buildProfile, if it's not there it breaks android tests

= SpecialPowersObserver.js =
* Removed the if aSubject.wrappedJSObject clause around calling the frame script.  It was an old hold-over from before I made this work in IPC mode.
* Added browser.tabs.remote check to determine if we are in IPC mode or not
* Made the load of the child frame script delay if we are in ipc mode and not delay if we are not in ipc mode.  I think delaying when in in-process mode was causing the intermittent windows failure that we were seeing.  For ipc mode, we must delay or it won't work.

= content/specialpowers.js =
* Added the "setPref" hack to determine if we are loaded inprocess or not
* if inprocess, we do not instantiate the content side observer, this prevents the test slow-downs we were seeing.
* for ipc, we do instantiate the content side observer

= tests/test_SpecialPowersExtension.html=
* Made the test wait for load.  By doing this I managed to make the intermittent failure I was seeing on windows reproducible, and that's how I determined that it was the delay setting in components/SpecialPowersObserver causing issues.  Right now this test is using an onload and the second specialpowers test does not use an on-load.  Both mechanisms should pass (as far as I know, though without the onload we are definitely racy), so it is good to test both.

Comment 55

[cmtalbert]

Well, guys, so much for my thoughts.  Still fails on windows 7 debug builds, but only on tinderbox.

Failing:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291949197.1291951675.2567.gz&fulltext=1#err0

Passing:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291938187.1291940385.12323.gz&fulltext=1

It's green everywhere else, which is nice.

Going back to try with a whole lot more debugging output.  But I think what is happening here is something I noticed when debugging today - there are times when you can ask message manager to load a child script for you and it simply doesn't.  I'm chatting with smaug about that in an email, we'll see if that turns out to be the culprit.  But why it only happens on this one OS is beyond me.  I'm also asking releng to see if I can get access to one of these slaves to run it directly.  Until then, it's back to try with 110% more debug output.

Comment 56

[cmtalbert]

Attachment: Version 5

So, I went back through the documentation of message manager last night.  And while reading I saw that once LoadFrameScript is called, then that frame script should be loaded for any new browser.  This made me think that perhaps the issue here is that my code was flooding the nsIFrameMessageManager's script list with a ton of links to the special powers frame script (specialpowers.js).  

So, I took some action to limit the loading of the frame script.  In specialpowersobserver.js you can now see that we ensure the script is only loaded once and we wait to get our chrome window created notification before doing so.

Next, since I now only register one frame script, I need the content script to operate the content-document-global-created observer.  However, I know we don't want to register too many observers as that really gets bad fast (timeouts, oranges go bonkers etc).  So, I gate the observer registration itself on whether or not we already have a specialpowers object assigned to the window.  And just to prevent us from constantly re-attaching specialpowers to windows, I also gate the attachToWindow function on the same check.  

With these changes, this patch (with a lot of debugging output) went **TOTALLY GREEN** on try last night.  I'm running this patch on try right now.

Comment 57

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

Comment on attachment 497515 [details] [diff] [review]
Version 5


>+      // Load the frame script into the window, if in IPC, delay load
Seems like a left-over from previous patch.

>+  }catch(ex) {
space after }


>+// NOTE: The observers are GC'd when the window dies, so while this looks like it should
>+// leak, it actually doesn't. And if you add in an observer for dom-window-destroyed or 
>+// xpcom-shutdown and upon capturing either of those events you unregister the 
Nit, those aren't events but notifications


Looks ok to me. 
Did you test this all with in-process mm and content process mm?

Comment 58

[cmtalbert]

(In reply to comment #57)
> Comment on attachment 497515 [details] [diff] [review]
> Version 5

Made the changes you requested.  Also made a change suggested by Josh, where I modified attachToWindow to take a parameter which is the subject of the content-document-global-created notification.  This way we can use calls of the form "SpecialPowers.foo()" in embedded iframe tests.
I changed the SpecialPowers tests to use this form (rather than "content.window.SpecialPowers.foo()"

> Looks ok to me. 
> Did you test this all with in-process mm and content process mm?
Yes, I always test in both firefox and fennec desktop build.  It runs in both.  I'll submit an updated patch.

Comment 59

[cmtalbert]

Attachment: An Updated Patch, addresses comments

Comment 60

[cmtalbert]

Pushed to m-c, crossing fingers: http://hg.mozilla.org/mozilla-central/rev/14ae20ed2bd5