1142279 - DataView should require `new`

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kelsey Gilbert [:jgilbert]]

The test requires that `DataView()` throw an exception:

var expr = "DataView(new ArrayBuffer)";
    // Use try/catch instead of calling shouldThrow to avoid different exception message being reported from different platform.
    try {
        TestEval(expr);
        testFailed(expr + " does not throw exception");
    } catch (e) {
        testPassed(expr + " threw exception");
    }

I'm told this is an easy fix with a longish tail of tests to fixup, since this non-spec syntax has infected our tests.

If I could get a JS person to write the engine fix, I am willing to deal with the test fixups.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: This is a plausible approach for the JS side of things

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: 0001-Forbid-non-new-constructors.-from-waldo.patch

Comment 3

[Kelsey Gilbert [:jgilbert]]

Attachment: 0002-Fix-JS-tests.patch

Comment 4

[Kelsey Gilbert [:jgilbert]]

Try looks good?:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8781fe15317

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8602296 [details] [diff] [review]
0001-Forbid-non-new-constructors.-from-waldo.patch

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

ThorwIfNotConstructing makes a whole lot more sense than ReportIsNotFunction... r=me

::: js/src/vm/NativeObject-inl.h
@@ +612,5 @@
>                                          JSMSG_BUILTIN_CTOR_NO_NEW, builtinName);
>  }
>  
> +inline bool
> +ThrowIfNotConstructing(JSContext *cx, const CallArgs &args, const char *builtinName)

Up till now, this has just been written out inline. Can we file a followup to do the trudging task of replacing the other places where this would be useful? The Proxy and TypedObject constructors come to mind, offhand. also this normally uses JSMSG_NOT_FUNCTION, which I always thought sucked, but there we are. We should at least endeavor to have a unified approach here.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8602298 [details] [diff] [review]
0002-Fix-JS-tests.patch

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

APPROVED.

Comment 7

[Kelsey Gilbert [:jgilbert]]

(In reply to Eric Faust [:efaust] from comment #5)
> Comment on attachment 8602296 [details] [diff] [review]
> 0001-Forbid-non-new-constructors.-from-waldo.patch
> 
> Review of attachment 8602296 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ThorwIfNotConstructing makes a whole lot more sense than
> ReportIsNotFunction... r=me
> 
> ::: js/src/vm/NativeObject-inl.h
> @@ +612,5 @@
> >                                          JSMSG_BUILTIN_CTOR_NO_NEW, builtinName);
> >  }
> >  
> > +inline bool
> > +ThrowIfNotConstructing(JSContext *cx, const CallArgs &args, const char *builtinName)
> 
> Up till now, this has just been written out inline. Can we file a followup
> to do the trudging task of replacing the other places where this would be
> useful? The Proxy and TypedObject constructors come to mind, offhand. also
> this normally uses JSMSG_NOT_FUNCTION, which I always thought sucked, but
> there we are. We should at least endeavor to have a unified approach here.

Can you file a follow-up bug for this?

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ded1da5b0d7a

Comment 10

[Florian Scholz (Open Web Docs)]

Release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/40#JavaScript

Site compat:
https://developer.mozilla.org/en-US/Firefox/Releases/40/Site_Compatibility#TypeError_when_DataView_called_without_new

Reference:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DataView#Browser_compatibility

Comment 11

[Eric Faust [:efaust]]

(In reply to Jeff Gilbert [:jgilbert] from comment #7)
> (In reply to Eric Faust [:efaust] from comment #5)
> > Comment on attachment 8602296 [details] [diff] [review]
> > 0001-Forbid-non-new-constructors.-from-waldo.patch
> > 
> > Review of attachment 8602296 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ThorwIfNotConstructing makes a whole lot more sense than
> > ReportIsNotFunction... r=me
> > 
> > ::: js/src/vm/NativeObject-inl.h
> > @@ +612,5 @@
> > >                                          JSMSG_BUILTIN_CTOR_NO_NEW, builtinName);
> > >  }
> > >  
> > > +inline bool
> > > +ThrowIfNotConstructing(JSContext *cx, const CallArgs &args, const char *builtinName)
> > 
> > Up till now, this has just been written out inline. Can we file a followup
> > to do the trudging task of replacing the other places where this would be
> > useful? The Proxy and TypedObject constructors come to mind, offhand. also
> > this normally uses JSMSG_NOT_FUNCTION, which I always thought sucked, but
> > there we are. We should at least endeavor to have a unified approach here.
> 
> Can you file a follow-up bug for this?

evilpie just did this in bug 1215814 :)