981036 - Disallow calling WebIDL constructors as functions for system callers in non-release builds

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

+++ This bug was initially created as a clone of Bug #979591 +++

Let's just do this in nightly/aurora for now

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Disallow calling DOM constructors as functions in non-release builds.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8387794 [details] [diff] [review]
Disallow calling DOM constructors as functions in non-release builds.

Review of attachment 8387794 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ +1200,5 @@
> +        name = self._ctor.identifier.name
> +        if name != "constructor":
> +            ctorName = name
> +        else:
> +            ctorName = self.descriptor.interface.identifier.name

I don't quite follow this part, or why you need to pass this name to CGMethodCall, but I'll defer to you on this one.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I don't quite follow this part, or why you need to pass this name to CGMethodCall

Just to get nicer error reporting.  For unnamed webidl constructors, identifier.name is "constructor"; for named ones it's the actual name.  So for Event if I want "Event" instead of "constructor" I need self.descriptor.interface.identifier.name, but for Image if I want "Image" instead of "HTMLImageElement" I need self._ctor.identifier.name.

And CGMethodCall uses this string for some error reporting as well (e.g. the "not enough arguments" case), so I figured I might as well share it.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> > I don't quite follow this part, or why you need to pass this name to CGMethodCall
> 
> Just to get nicer error reporting.  For unnamed webidl constructors,
> identifier.name is "constructor"; for named ones it's the actual name.  So
> for Event if I want "Event" instead of "constructor" I need
> self.descriptor.interface.identifier.name, but for Image if I want "Image"
> instead of "HTMLImageElement" I need self._ctor.identifier.name.

I see. It might be nice to comment that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Added a comment, and pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/2045471633ac

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Added a comment

Except failed to qref.

https://hg.mozilla.org/integration/mozilla-inbound/rev/da877497bf3e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2045471633ac
https://hg.mozilla.org/mozilla-central/rev/da877497bf3e

Comment 8

[Kris Maglione [:kmag]]

Hm. Is there some reason that this can't be a warning rather than an error for at least one release cycle? This is going to break a lot of add-ons.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Kris, you mean _has_ broken a lot of addons?  This is already on all the channels where it matters...  Have there been reports of broken addons?  I'd love to see them!

Are you perhaps confusing this bug and bug 979591?

Comment 10

[Kris Maglione [:kmag]]

Well, it's broken several of my add-ons. It's hard to find out how many others are affected from MXR, in this case, but given how long this was allowed, and the fact that even core code was broken by it, it would have been nice to start with a warning rather than an error in the first place.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That's fair, but note that this is why the patch is nightly-and-aurora only...  The idea was to find addons that are affected and get them fixed.  Doing that with warnings has not been very effective in the past, unfortunately.  :(

Comment 12

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> That's fair, but note that this is why the patch is nightly-and-aurora
> only...  The idea was to find addons that are affected and get them fixed. 
> Doing that with warnings has not been very effective in the past,
> unfortunately.  :(

Yeah. The kind of addons authors that respond to deprecation warnings are also the kind that instant fix their addons when they're broken. And it's not like this requires any complicated refactoring - it's a purely syntactic change.