Closed Bug 1142279 Opened 5 years ago Closed 5 years ago

DataView should require `new`

Categories

(Core :: JavaScript Engine, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

()

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(3 files)

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.
Blocks: 1128035
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Blocks: 1062075
Assignee: nobody → jgilbert
Attachment #8602296 - Flags: review?(efaustbmo)
Attachment #8602298 - Flags: review?(efaustbmo)
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.
Attachment #8602296 - Flags: review?(efaustbmo) → review+
Comment on attachment 8602298 [details] [diff] [review]
0002-Fix-JS-tests.patch

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

APPROVED.
Attachment #8602298 - Flags: review?(efaustbmo) → review+
(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?
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/mozilla-central/rev/ded1da5b0d7a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Keywords: site-compat
(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 :)
Flags: needinfo?(efaustbmo)
You need to log in before you can comment on or make changes to this bug.