Closed Bug 453331 Opened 13 years ago Closed 13 years ago

Quick stubs: handle members with the same name

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(2 files)

The general problem is, there are cases where two interfaces have scriptable members with the same name and there's a class that implements both interfaces.  In these cases, XPConnect first calls NewResolve and lets the scriptable helper (if any) sort it out; and failing that, it exposes whichever member shows up first in the classinfo.

Quick stubs don't have access to scriptable helpers or classinfo, so we have to be careful about stubbing these cases.

Examples:

Two scriptable methods from different interfaces, same name, different signatures.

  nsIDOMEventTarget::AddEventListener(
      string, nsIDOMEventListener, boolean);
  nsIDOMNSEventTarget::AddEventListener(
      string, nsIDOMEventListener, boolean, boolean);

  imgIDecoderObserver::OnStartRequest(imgIRequest);
  nsIRequestObserver::OnStartRequest(nsIRequest, nsISupports*);

  imgIDecoderObserver::OnStopRequest(imgIRequest, boolean);
  nsIRequestObserver::OnStopRequest(nsIRequest, nsISupports, nsresult);

  string nsIDOMJSWindow::Prompt();
  string nsIDOMWindowInternal::Prompt(
      string, string, string, unsigned int);

  nsIDOMDocument nsIDOMWindow::GetDocument();
  nsIDOMDocumentView nsIDOMAbstractView::GetDocument();

Two IDL attributes, same name; one is readonly and one is not.  Bug 453105 and the bug described in bug 407216 comment 106 are both special cases of this.

  nsIDOMHTMLCollection.length (readonly)
  nsIDOMHTMLOptionsCollection.length

  nsIDOMHTMLOptionElement.text (readonly)
  nsIDOMNSHTMLOptionElement.text

Potentially, multiple interfaces implemented by a class might have methods with the same name and signature, but different implementations.  There are lots of cases where a class implements two unrelated interfaces that both have a method (of which a few are listed here) but as far as I know, every class that implements both implements them with a single method.  So there's no actual conflict.  (IIRC, C++ makes it possible but convoluted to implement them differently.)
  nsIDOMNode nsIDOMNodeList::Item(unsigned int);
  nsIDOMNode nsIDOMHTMLCollection::Item(unsigned int);

  unsigned int nsIDOMNodeList::GetLength();
  unsigned int nsIDOMHTMLCollection::GetLength();

  boolean nsIEditor::CanDrag(nsIDOMEvent);
  boolean nsIHTMLEditor::CanDrag(nsIDOMEvent);

Potentially, an XPCOM object might implement multiple interfaces via tearoffs that could have this issue.  I don't know an easy way to find these cases.  Not being able to find all the cases is scary.
> Potentially, multiple interfaces implemented by a class might have methods with
> the same name and signature, but different implementations.

Anything like that would already be broken when used from JS (no way to indicate which method you "really" want), so I don't think we should worry about it.
(In reply to comment #1)
> > Potentially, multiple interfaces implemented by a class might have methods with
> > the same name and signature, but different implementations.
> 
> Anything like that would already be broken when used from JS (no way to
> indicate which method you "really" want), so I don't think we should worry
> about it.

Right.  A scriptable helper could make it nonbroken.  But then there shouldn't be quick stubs for anything that has special magic scriptable-helper support.
(Clarifying comment #0)
> Quick stubs don't have access to scriptable helpers or classinfo,
> so we have to be careful about stubbing these cases.

I meant the stuff that generates quick stubs at compile time.  (We don't want quick stubs to call scriptable helpers or examine classinfo at run time either, of course.)
Attached patch static analysisSplinter Review
This treehydra script checks for name conflicts in classes that implement multiple interfaces.  All known potential conflicts are listed.

(This patch disables the stack.js analysis because the tree currently can't pass that one-- it stops the build with errors.)
Assignee: nobody → jorendorff
Attached patch v1Splinter Review
This also deletes an obsolete comment in qsgen.py. (That bug was fixed before quickstubs landed.)
Attachment #337286 - Flags: superreview?(jst)
Attachment #337286 - Flags: review?(jst)
Attachment #337286 - Flags: superreview?(jst)
Attachment #337286 - Flags: superreview+
Attachment #337286 - Flags: review?(jst)
Attachment #337286 - Flags: review+
Pushed http://hg.mozilla.org/mozilla-central/rev/39e07e16d8f8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 337286 [details] [diff] [review]
v1

>+    # Caution: nsGlobalWindow also implements nsIDOMAbstractView, which *also*
>+    # has an attribute named document (of a different type!).  The two getters
>+    # behave differently.
Is this behaviour difference a bug?
Probably not: the nsIDOMDocument behavior is there for web compat, and since web code doesn't call the other overload we're OK.
You need to log in before you can comment on or make changes to this bug.