299967 - Cached wrappers with QI state create hard-to-reproduce bugs

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We had some Novell chrome that would occasionally fail in hard to reproduce
ways. It turned out to be due to the following sort of code:

var psvc = Components.classes["@mozilla.org/preferences-service;1"]
             .getService(Components.interfaces.nsIPrefBranch);
var branch = psvc.getBranch(null);

That code is obviously incorrect and should fail. But it usually worked, because
usually there was a previously executed fragment of Javascript (entirely
unrelated in terms of program structure) like this:

var service = Components.classes["@mozilla.org/preferences-service;1"]
             .getService(Components.interfaces.nsIPrefService);

I assume that that caused the pref-service wrapper to note that it implements
nsIPrefService, and if the first fragment was lucky enough to find that wrapper,
then it succeeded, otherwise failed.

This sort of unpredictable behaviour is obviously very undesirable from a
developer's point of view.

I don't know much about this layer of our system, but I suggest we should not
allow the wrapper to be reused in this situation. Perhaps we could still allow
wrapper reuse for objects that support flattening.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that we have all sorts of code that assumes that flattening will happen....

Comment 2

[Mike Shaver (:shaver emeritus)]

We don't have that much that expects that it'll get back a wrapper from
getService which has already had another interface added on, I bet.

Two solutions that wouldn't break much, if any, code:
- don't reuse wrappers for getService, unless the wrapper has only the interface
  specified in the call, or the class has classinfo; or
- add classinfo to everything.

(Pretty sure it would break no code in our tree, and I think the little bits of
code elsewhere it might break would be more than happy to make the easy fixes.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ah, if the suggestion is to not reuse wrappers for GetService, then that's all
good by me.

Comment 4

[[e-mail]]

(In reply to comment #1)
>Note that we have all sorts of code that assumes that flattening will happen....
Although only by explicit calls to instanceof or QueryInterface.
It shouldn't be necessary to search for existing wrappers for arbitrary XPCOM
calls, except probably for objects with class info.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Couldn't this happen apart from getService, wherever we have an XPCOM object
being transmitted to Javascript via multiple native calls?

Why can't we just disallow wrapper reuse unless the object has classinfo?

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #5)
> Couldn't this happen apart from getService, wherever we have an XPCOM object
> being transmitted to Javascript via multiple native calls?

Yes.

> Why can't we just disallow wrapper reuse unless the object has classinfo?

Because hardly anything implements nsIClassInfo (which violates XPCOM rules, btw
-- a singleton per class returned by QI).  We'd break a bunch of script that
counts on this feature.

I'm in favor of making things work more often, not less often.  We should make
getService not reuse wrappers except under the two conditions shaver listed, and
we'll probably want to change a few services to implement nsIClassInfo.

Anyone want to take this bug from me?  dbradley may be too busy right now, so
don't anyone be shy ;-).

/be

Comment 7

[Brendan Eich [:brendan]]

I'm a poor owner for this bug. Throwing it back, someone else should feel free to fish for a fix.

/be

Comment 8

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.