607767 - test-observer-service fails on "observer-service.add() should pass subject (({}) != ({}))"

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

On the latest Firefox trunk nightly, test-observer-service fails on "observer-service.add() should pass subject (({}) != ({}))".

--------------------------------------------------------------------------------
(addon-sdk)myk@myk:~/Projects/addon-sdk/packages/jetpack-core$ cfx test -F observer-service
Using binary at '/home/myk/bin/firefox'.
Using profile at '/tmp/tmpjxbBsm.mozrunner'.

(firefox-bin:23173): GLib-WARNING **: g_set_prgname() called multiple times
......error: TEST FAILED: test-observer-service.testObserverService (failure)
error: fail: observer-service.add() should pass subject (({}) != ({}))
info: Traceback (most recent call last):
  File "resource://jetpack-core-jetpack-core-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 255, in anonymous
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 280, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 298, in start
    this.test.testFunction(this);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 63, in runTest
    test(runner);
  File "resource://jetpack-core-jetpack-core-tests/test-observer-service.js", line 55, in anonymous
    "observer-service.add() should pass subject");
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 227, in assertEqual
    this.fail(message);
  File "resource://jetpack-core-jetpack-core-lib/unit-test.js", line 145, in fail
    console.trace();
.....
11 of 12 tests passed.
FAIL
Total time: 1.513529 seconds
Program terminated unsuccessfully.
--------------------------------------------------------------------------------

This looks like one of the failures in bug 607764 and another in bug 607113, suggesting a common cause.

Atul, Peter: any thoughts on what that common cause might be?

Comment 1

[Johnny Stenback (:jst)]

Myk, can you explain how this can be reproduced? Where should one pull from,
what needs to be built, etc etc?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Pull from http://github.com/mozilla/addon-sdk or its Hg mirror at
https://hg.mozilla.org/labs/jetpack-sdk/.  Then enter your local clone's
directory in a terminal and run "source bin/activate" ("bin\activate.bat" on
Windows).  Then enter packages/jetpack-core/ and run "cfx test -F symbiont" or
"cfx test -F observer-service".

That command will look for Firefox, but if it doesn't find it (or if you think
it might be finding a different binary than the one you want to test with),
specify "--binary=/path/to/binary" on that command line.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Erm, copy/paste fail; I meant to say that you should run "cfx test -F observer-service" as the last step.

Comment 4

[Drew Willcoxon :adw]

Peter, this is more sandbox inconsistency.  In this case, an nsIURI is being passed as the subject to nsIObserverService.notifyObservers outside a sandbox.  Inside the sandbox, an nsIObserver grabs this subject, and outside the sandbox the original nsIURI is compared to the subject grabbed inside the sandbox using ==, and they're not equal.

e.g.:

  var Cc = Components.classes;
  var Ci = Components.interfaces;
  var sp = Cc["@mozilla.org/systemprincipal;1"].
           createInstance(Ci.nsIPrincipal);
  var s = Components.utils.Sandbox(sp);
  Components.utils.evalInSandbox(
    'var global = this;' +
    'var os = Components.classes["@mozilla.org/observer-service;1"].' +
    '         getService(Components.interfaces.nsIObserverService);' +
    'os.addObserver({' +
    '   observe: function (subject) global.subject = subject' +
    '}, "foo", false);',
    s
  );
  var subject = Cc["@mozilla.org/network/io-service;1"].
                getService(Ci.nsIIOService).
                newURI("http://www.foo.com", null, null);
  Cc["@mozilla.org/observer-service;1"].
    getService(Ci.nsIObserverService).
    notifyObservers(subject, "foo", "");
  s.subject == subject; /* false */

Incidentally, s.subject is simply an nsISupports.  It has to be QI'ed to nsIURI.

Here's an example without sandboxes that works as expected:

  var Cc = Components.classes;
  var Ci = Components.interfaces;
  var os = Cc["@mozilla.org/observer-service;1"].
           getService(Ci.nsIObserverService);
  var osSubject = null;
  os.addObserver({
    observe: function (subject) osSubject = subject
  }, "foo", false);
  var subject = Cc["@mozilla.org/network/io-service;1"].
                getService(Ci.nsIIOService).
                newURI("http://www.foo.com", null, null);
  os.notifyObservers(subject, "foo", "");
  osSubject == subject; /* true */

Here, osSubject is already an nsIURI and doesn't need to be QI'ed.

Comment 5

[Johnny Stenback (:jst)]

With the fixes for bugs 607799, 607863, 606585, and bug 608142 this still fails
with the same message as in comment 0.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

The problem is that we thought that the URI object was an outer window because NoHelper objects had an innerObject hook (!). This removes the ability for XPConnect objects to confuse us like that any more.

Comment 7

[Johnny Stenback (:jst)]

With mrbkap's patch applied the observer-service tests pass.

Comment 8

[Johnny Stenback (:jst)]

http://hg.mozilla.org/mozilla-central/rev/1c265164a571

Comment 9

[(no longer active)]

This patch caused the Mochitest-5 boxes to go orange on Linux opt and Linux64 opt (other platforms pending right now).  I haven't backed the patch out, but I'm reopening this bug to alert the right people.

Comment 10

[Johnny Stenback (:jst)]

Backed out:

http://hg.mozilla.org/mozilla-central/rev/b82c2f90001c

Comment 11

[(no longer active)]

This also seems to have caused another failure on osx:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288552231.1288554189.10288.gz

Comment 12

[(no longer active)]

...and a similar failure on Windows:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1288552721.1288555720.16234.gz

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Ugh, nsStandardURL claims to implement nsIClassInfo but doesn't actually return any interfaces from GetInterfaces (it only wants the MAIN_THREAD_ONLY flag); so the new XPCWrappedNative doesn't actually know that it implements nsIURI.

Comment 14

[Johnny Stenback (:jst)]

mrbkap's updated fix for this fixes this testcase (patch yet to be attached here :).

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v2

This patch attempts to detect this case and if the object's classinfo lies about the interfaces it implements, we just use the set from the old wrapped native. Note that the returned wrapped native *must* have nsISupports in its set.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v3

This fixes confusion with the history object and also fixes a webconsole test.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/9ec3b67b2fd7