Closed Bug 898359 Opened 7 years ago Closed 6 years ago

[binary data] non-scalar types like any, object, and string

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: nmatsakis, Assigned: nmatsakis)

References

Details

Attachments

(3 files, 1 obsolete file)

Currently the binary data patches only support scalar values. We need to include support for any, string, and object. We must be careful in these cases to prevent exposing typed array buffers for obvious security reasons.
Attached patch Bug898359.diff (obsolete) — Splinter Review
Extend type objects with base types String, Any, and Object, which support "by reference" values.
Attachment #824910 - Flags: review?(sphink)
Comment on attachment 824910 [details] [diff] [review]
Bug898359.diff

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

I apologize for the delay in getting to this. It was much clearer than I expected. Nice stuff.

Clearing review for now because of the question about the failing tests, and the cross-compartment wrapper stuff. Everything else is r+.

I read through the existing code before doing this review, so some additional comments:

diff --git a/js/src/builtin/TypedObject.cpp b/js/src/builtin/TypedObject.cpp
--- a/js/src/builtin/TypedObject.cpp
+++ b/js/src/builtin/TypedObject.cpp
@@ -224,12 +224,12 @@ TypeObjectToSource(JSContext *cx, unsign
  * - val = {x: 22, y: 44}
  * This would result in loading the value of `x`, converting
- * it to a float32, and hen storing it at the appropriate offset,
+ * it to a float32, storing it at the appropriate offset,

For this:
   public:
     // Returns the offset in bytes within the object where the `void*`
     // pointer can be found.
     static size_t dataOffset();

I couldn't figure out from the comment exactly what this was. What void*? But I guess it's the offset where the pointer to the data represented by this TypedDatum is stored? Sorry, it's probably obvious, but could you expand this comment a little? (Really what happened is that on first reading, I freaked out because it looked like this was assuming that the actual data was always stored inline in the JSObject*, which seemed patently untrue.)

What happens when you use this stuff through cross-compartment wrappers? I don't see any NonGenericMethodCall stuff going on. How is it supposed to work? If it has been decided and the code already implements it, can you test it? (Assuming I'm not just missing the tests.)

::: js/src/builtin/TypeRepresentation.cpp
@@ +80,5 @@
>  }
>  
>  bool
> +TypeRepresentationHasher::matchReferences(ReferenceTypeRepresentation *key1,
> +                                          ReferenceTypeRepresentation *key2)

Is this match* stuff the same as the "equivalent" stuff in the spec? Might be nice to use the same name, though it's long. equivReferences?

@@ +206,5 @@
> +{
> +    switch (type) {
> +      case TYPE_ANY:
> +        size_ = sizeof(js::HeapValue);
> +        alignment_ = alignof(js::HeapValue);

You sure alignof is portable? Why do we have JS_ALIGNMENT_OF? You might need to use JSVAL_ALIGNMENT instead.

@@ +212,5 @@
> +
> +      case TYPE_OBJECT:
> +      case TYPE_STRING:
> +        size_ = sizeof(js::HeapPtrObject);
> +        alignment_ = alignof(js::HeapPtrObject);

JS_ALIGNMENT_OF(js::HeapPtrObject) if it turns out to not be portable.

@@ +247,3 @@
>      fieldCount_(0) // see ::init() below!
>  {
> +    // note: size_, alignment_, and pod_ are computed in ::init() below

Do you get annoying warnings if you don't initialize these here? These initial values are kind of a lie, so I'd rather not have them at all unless they're needed for warning suppression.

@@ +679,5 @@
> +      {
> +        ArrayTypeRepresentation *arrayRepr = repr->asArray();
> +        TypeRepresentation *elementRepr = arrayRepr->element();
> +        for (size_t i = 0; i < arrayRepr->length(); i++) {
> +            visitReferences(elementRepr, mem, visitor);

I wonder if this is able to optimize away the whole loop if elementRepr->kind() == Scalar.

@@ +761,5 @@
> +///////////////////////////////////////////////////////////////////////////
> +// Tracing instances
> +
> +namespace js {
> +class MemoryTracingVisitor {

Nice and clean.

(Which makes me think -- it's sort of unfortunate to do all this scanning and recursing to find the markable fields. Would be nice to have a condensed vector of <offset,type> tuples per type that could be scanned directly. But never mind me. Not time for that type of tuning yet. And you'd still end up having to iterate over arrays.)

@@ +770,5 @@
> +    MemoryTracingVisitor(JSTracer *trace)
> +      : trace_(trace)
> +    {}
> +
> +    void visitReference(ReferenceTypeRepresentation *repr,

no line wrap needed

@@ +777,5 @@
> +} // namespace js
> +
> +void
> +js::MemoryTracingVisitor::visitReference(ReferenceTypeRepresentation *repr,
> +                                         uint8_t *mem)

no line wrap

::: js/src/builtin/TypeRepresentation.h
@@ +130,5 @@
>      // buffer, for use in error messages and the like.
>      bool appendString(JSContext *cx, StringBuffer &buffer);
>  
> +    // Initializes memory that contains an instance of this type
> +    // with appropriate default values (typically 0).

*an appropriate default value, I'd think.

::: js/src/builtin/TypedObject.cpp
@@ +112,4 @@
>  }
>  
>  static inline bool
> +IsSimpleTypeObject(JSObject &type)

I think of these as "leaf" types, but perhaps that's no more descriptive. We certainly don't want to call them "atoms", "atomic", or "primitive"!

I guess "simple" is fine.

@@ +186,5 @@
> +
> +    RootedObject thisObj(cx, ToObjectIfObject(args.thisv()));
> +    if (!thisObj || !IsTypeObject(*thisObj)) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage,
> +                             nullptr, JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS,

Could this be unified with JSMSG_TYPED_ARRAY_BAD_ARGS, JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS, JSMSG_PAR_ARRAY_BAD_ARG? At least the 1st one. The latter two, probably not.

@@ +398,5 @@
> +          return true;                                                        \
> +      }
> +
> +        JS_FOR_EACH_SCALAR_TYPE_REPR(SCALARTYPE_CALL)
> +#undef BINARYDATA_SCALAR_DEFINE

Mismatched macro name

@@ +416,5 @@
> + */
> +
> +const Class js::ReferenceType::class_ = {
> +    "Reference",
> +    JSCLASS_HAS_RESERVED_SLOTS(JS_TYPEOBJ_SCALAR_SLOTS),

This should either be JS_TYPEOBJ_SIMPLE_SLOTS or JS_TYPEOBJ_REFERENCE_SLOTS. (Or JS_TYPEOBJ_LEAF_SLOTS.)

@@ +792,5 @@
>  {
>      TypeRepresentation *typeRepr =
>          TypeRepresentation::fromOwnerObject(*typeReprOwnerObj);
>  
> +    if (typeRepr->pod()) {

Hm... is pod() the same as "scalar"? Or should this use the "opaque" spec language? Well, I guess pod() makes the most sense for most of its uses.

@@ +816,4 @@
>      }
>  
>      // variable -- always false since we do not yet support variable-size types
>      RootedValue variable(cx, JSVAL_FALSE);

Wow, we really don't have TrueValue() and FalseValue()? Ugh. Filed bug 937916.

@@ +1249,4 @@
>          return false;
>  
> +    numFun->initReservedSlot(JS_TYPEOBJ_SLOT_TYPE_REPR,
> +                             ObjectValue(*typeReprObj));

no line wrap needed

@@ +1310,5 @@
> +    // Create ctor itself
> +
> +    RootedFunction ctor(cx,
> +                        global->createConstructor(cx, T::construct,
> +                                                  className, 2));

what's 2? Oh, "length". Ugh. Sorry, but can you name this with something like

  const int constructorLength = 2;

on the line before?

@@ +1585,5 @@
>          gc::MarkSlot(trace, &object->getReservedSlotRef(i), "TypedObjectSlot");
> +
> +    uint8_t *mem = TypedMem(*object);
> +    TypeRepresentation *repr = typeRepresentation(*GetType(*object));
> +    if (!repr->pod())

Oh, hm. Maybe pod() *is* the right name.

@@ +2259,5 @@
>          return nullptr;
>  
>      TypeRepresentation *typeRepr = typeRepresentation(*type);
>      size_t memsize = typeRepr->size();
> +    uint8_t *memory = (uint8_t*) JS_malloc(cx, memsize);

Seems better to call cx->malloc_(memsize) directly.

::: js/src/builtin/TypedObject.h
@@ +109,2 @@
>    public:
> +    static const Class class_;

Seems like you should have a comment like:

// Type for scalar type constructors like `uint8`. All scalar type constructors
// share a common js::Class and JSFunctionSpec. Scalar types [FIXME] are
// non-opaque (their storage is visible unless combined with an opaque
// reference type.)

...except I probably have the wrong terminology. Also, can you comment why the js::Class is shared? (Especially given that it previously wasn't.)

@@ +114,4 @@
>      static bool call(JSContext *cx, unsigned argc, Value *vp);
>  };
>  
> +// Type for reference type constructors like `Any`

Expand to something similar to this?:

// Type for reference type constructors like `Any`, `Object`, or `string`. All reference type
// constructors share a common js::Class and JSFunctionSpec. Reference types
// [FIXME] are opaque, meaning the user cannot access their storage
// representation.

@@ +434,5 @@
>  
>  /*
> + * Usage: StoreReference(targetDatum, targetOffset, value)
> + *
> + * Intrinsic function. Stores value (which must be an int32 or uint32)

cut & paste error: value will certainly *not* be an int32/uint32.

Also, there's something missing in this sentence and all other occurrences: "Stores value by scalarTypeRepr". I think you meant "...represented by..." or something. Duplicated in all versions of this comment.

And in the StoreScalar case, why must it be int32/uint32? The code says it just has to be .isNumber. How do float32/float64 work?

@@ +461,5 @@
>  #define JS_LOAD_SCALAR_CLASS_DEFN(_constant, T, _name)                        \
>  class LoadScalar##T {                                                         \
>    public:                                                                     \
>      static bool Func(ThreadSafeContext *cx, unsigned argc, Value *vp);        \
>      static const JSJitInfo JitInfo;                                           \

Where is the JSJitInfo thing commented? (Just the 1-line, basic idea.)

::: js/src/builtin/TypedObjectConstants.h
@@ +20,5 @@
>  // Slots on all type objects
>  #define JS_TYPEOBJ_SLOT_TYPE_REPR          0  // Associated Type Representation
>  
>  // Slots on scalars
>  #define JS_TYPEOBJ_SCALAR_SLOTS            1  // Maximum number

Either make this _SIMPLE_ or add a _REFERENCE_ version.

@@ +78,5 @@
>  #define JS_SCALARTYPEREPR_UINT8_CLAMPED 8
>  
> +// These constants are for use exclusively in JS code. In C++ code,
> +// prefer ReferenceTypeRepresentation::TYPE_ANY etc, since that allows
> +// you to write a switch which will receive a warning if you omit a

Nit: s/which/that/ (yes, I'm the *only* one who will complain about this particular English grammar thing. Blame it on my thesis adviser.)

@@ +80,5 @@
> +// These constants are for use exclusively in JS code. In C++ code,
> +// prefer ReferenceTypeRepresentation::TYPE_ANY etc, since that allows
> +// you to write a switch which will receive a warning if you omit a
> +// case.
> +#define JS_REFERENCETYPEREPR_ANY        0

JS_REFERENCETYPE_REPR_ANY, maybe?

Also, would it be bad to make everything in this file be const ints in namespace js instead?

::: js/src/tests/ecma_6/TypedObject/referencetypetrace.js
@@ +62,5 @@
> +
> +  TestArrayElements(Object);
> +  TestArrayElements(Any);
> +
> +  // FIXME -- for some reason I do not understand the following tests fail:

That seems worrisome. Any ideas by now?
Attachment #824910 - Flags: review?(sphink)
(In reply to Steve Fink [:sfink] from comment #2)
> What happens when you use this stuff through cross-compartment wrappers? I
> don't see any NonGenericMethodCall stuff going on. How is it supposed to
> work? If it has been decided and the code already implements it, can you
> test it? (Assuming I'm not just missing the tests.)

I don't know. That sounds like a separate bug is warranted to me.

> Is this match* stuff the same as the "equivalent" stuff in the spec? Might
> be nice to use the same name, though it's long. equivReferences?

I named it `match` because that's the name used by the Hashtable code,
but yes it's the same as the equivalence relation. I'm inclined
to leave the name as is, though.

> You sure alignof is portable? Why do we have JS_ALIGNMENT_OF? You might need
> to use JSVAL_ALIGNMENT instead.

No, I'm in fact sure it's not. Good catch. I've been instructed to use MOZ_ALIGNOF in the past. Is that ok?

> @@ +247,3 @@
> >      fieldCount_(0) // see ::init() below!
> >  {
> > +    // note: size_, alignment_, and pod_ are computed in ::init() below
> 
> Do you get annoying warnings if you don't initialize these here? These
> initial values are kind of a lie, so I'd rather not have them at all unless
> they're needed for warning suppression.

I don't initialize them there, but I do pass dummy values to
the superconstructor. I guess I could add an alternate constructor
that does no initialization, just for this purpose. Would you prefer that?
I'd sort of rather not since it makes it possible to use that constructor without
realizing the distinction, but it is true that the current situation is also
non-ideal.

> @@ +679,5 @@
> > +      {
> > +        ArrayTypeRepresentation *arrayRepr = repr->asArray();
> > +        TypeRepresentation *elementRepr = arrayRepr->element();
> > +        for (size_t i = 0; i < arrayRepr->length(); i++) {
> > +            visitReferences(elementRepr, mem, visitor);
> 
> I wonder if this is able to optimize away the whole loop if
> elementRepr->kind() == Scalar.

We do check for pod() up above, so the code won't even get to this loop in that case.

> > +    // Initializes memory that contains an instance of this type
> > +    // with appropriate default values (typically 0).
> 
> *an appropriate default value, I'd think.

No, because this may be a compound type like a struct where there are multiple values to be initialized.

> @@ +186,5 @@
> > +
> > +    RootedObject thisObj(cx, ToObjectIfObject(args.thisv()));
> > +    if (!thisObj || !IsTypeObject(*thisObj)) {
> > +        JS_ReportErrorNumber(cx, js_GetErrorMessage,
> > +                             nullptr, JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS,
> 
> Could this be unified with JSMSG_TYPED_ARRAY_BAD_ARGS,
> JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS, JSMSG_PAR_ARRAY_BAD_ARG? At least the
> 1st one. The latter two, probably not.

JSMSG_TYPED_ARRAY_BAD_ARGS just says "invalid arguments", whereas this message does
tell you which argument was invalid.

> @@ +792,5 @@
> >  {
> >      TypeRepresentation *typeRepr =
> >          TypeRepresentation::fromOwnerObject(*typeReprOwnerObj);
> >  
> > +    if (typeRepr->pod()) {
> 
> Hm... is pod() the same as "scalar"? Or should this use the "opaque" spec
> language? Well, I guess pod() makes the most sense for most of its uses.

I will rename to opaque to match the spec. It's not precisely the same as POD in C++
(i.e., in C++ a Foo* is "POD"). 
 
> ...except I probably have the wrong terminology. Also, can you comment why
> the js::Class is shared? (Especially given that it previously wasn't.)

I changed to a shared js::Class just because it made the code simpler (less template magic). There is no real performance implication, I don't think, particularly since I plan to optimize calls to these constructors in ion code.

> Where is the JSJitInfo thing commented? (Just the 1-line, basic idea.)

I'm not sure.

> @@ +80,5 @@
> > +// These constants are for use exclusively in JS code. In C++ code,
> > +// prefer ReferenceTypeRepresentation::TYPE_ANY etc, since that allows
> > +// you to write a switch which will receive a warning if you omit a
> > +// case.
> > +#define JS_REFERENCETYPEREPR_ANY        0
> 
> JS_REFERENCETYPE_REPR_ANY, maybe?

I'm not sure why I adopted the convention of JS_XXXTYPEREPR_YYY vs JS_XXX_TYPE_REPR_YYY but I did use it consistently for the other similar constants, though not all across the file it seems. I think I wanted the `_` to denote the logical groupings vs the word groupings. But maybe it just makes it hard to read. I can clean this up.

> Also, would it be bad to make everything in this file be const ints in
> namespace js instead?

The reason they are #define's is because they are used in TypedObject.js, which is processed by cpp, but which is still JavaScript.

> That seems worrisome. Any ideas by now?

I found that CountHeap is not especially reliable. Stepping through the tracing code, it is doing the right thing. I have patch that modifies CountHeap to permit more reliable testing, I'll post the patch and have you review it, because it's nontrivial (though simple). Given that modified patch, the test passes (I'll attach the updated test too).
Attached patch Bug898359.diffSplinter Review
Address nits from sfink (not test failures, those come later)
Attachment #824910 - Attachment is obsolete: true
Attachment #832303 - Flags: review+
Add ability to test whether object A can reach object B.
Attachment #832305 - Flags: review?(sphink)
Update test to use new `countHeap` parameter. It passes now.
Attachment #832307 - Flags: review?(sphink)
(In reply to Niko Matsakis [:nmatsakis] from comment #3)
> (In reply to Steve Fink [:sfink] from comment #2)
> > What happens when you use this stuff through cross-compartment wrappers? I
> > don't see any NonGenericMethodCall stuff going on. How is it supposed to
> > work? If it has been decided and the code already implements it, can you
> > test it? (Assuming I'm not just missing the tests.)
> 
> I don't know. That sounds like a separate bug is warranted to me.

Ok. I think you'll need it. Right now, the nongeneric stuff is a horrible cut & paste boilerplate mess. I attempted to templatize it some in bug 921081, but it's still ugly, and I haven't gone back to fix it up.

But you're right; that's totally a separate bug since the existing stuff has the same problem. You're just adding some more instances.

> > Is this match* stuff the same as the "equivalent" stuff in the spec? Might
> > be nice to use the same name, though it's long. equivReferences?
> 
> I named it `match` because that's the name used by the Hashtable code,
> but yes it's the same as the equivalence relation. I'm inclined
> to leave the name as is, though.

Fine with me.

> > You sure alignof is portable? Why do we have JS_ALIGNMENT_OF? You might need
> > to use JSVAL_ALIGNMENT instead.
> 
> No, I'm in fact sure it's not. Good catch. I've been instructed to use
> MOZ_ALIGNOF in the past. Is that ok?

Yeah, that sounds better.

> > @@ +247,3 @@
> > >      fieldCount_(0) // see ::init() below!
> > >  {
> > > +    // note: size_, alignment_, and pod_ are computed in ::init() below
> > 
> > Do you get annoying warnings if you don't initialize these here? These
> > initial values are kind of a lie, so I'd rather not have them at all unless
> > they're needed for warning suppression.
> 
> I don't initialize them there, but I do pass dummy values to
> the superconstructor. I guess I could add an alternate constructor
> that does no initialization, just for this purpose. Would you prefer that?
> I'd sort of rather not since it makes it possible to use that constructor
> without
> realizing the distinction, but it is true that the current situation is also
> non-ideal.

Not worth the trouble.

> > @@ +679,5 @@
> > > +      {
> > > +        ArrayTypeRepresentation *arrayRepr = repr->asArray();
> > > +        TypeRepresentation *elementRepr = arrayRepr->element();
> > > +        for (size_t i = 0; i < arrayRepr->length(); i++) {
> > > +            visitReferences(elementRepr, mem, visitor);
> > 
> > I wonder if this is able to optimize away the whole loop if
> > elementRepr->kind() == Scalar.
> 
> We do check for pod() up above, so the code won't even get to this loop in
> that case.

Oh! You're right, you have this covered.

> > > +    // Initializes memory that contains an instance of this type
> > > +    // with appropriate default values (typically 0).
> > 
> > *an appropriate default value, I'd think.
> 
> No, because this may be a compound type like a struct where there are
> multiple values to be initialized.

It's arguable -- does a struct have a default value, or a set of default values [for its fields]? I'd say it has a default value which happens to be comprised of the default values of all of its fields. But sure, you can leave it.

> > @@ +186,5 @@
> > > +
> > > +    RootedObject thisObj(cx, ToObjectIfObject(args.thisv()));
> > > +    if (!thisObj || !IsTypeObject(*thisObj)) {
> > > +        JS_ReportErrorNumber(cx, js_GetErrorMessage,
> > > +                             nullptr, JSMSG_TYPEDOBJECT_HANDLE_BAD_ARGS,
> > 
> > Could this be unified with JSMSG_TYPED_ARRAY_BAD_ARGS,
> > JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS, JSMSG_PAR_ARRAY_BAD_ARG? At least the
> > 1st one. The latter two, probably not.
> 
> JSMSG_TYPED_ARRAY_BAD_ARGS just says "invalid arguments", whereas this
> message does
> tell you which argument was invalid.

I know. But your new message still doesn't say anything specific to typed objects. So I'm saying you could give this a more generic name and change (or file a bug to change) typed arrays to use the shared one.

> > @@ +792,5 @@
> > >  {
> > >      TypeRepresentation *typeRepr =
> > >          TypeRepresentation::fromOwnerObject(*typeReprOwnerObj);
> > >  
> > > +    if (typeRepr->pod()) {
> > 
> > Hm... is pod() the same as "scalar"? Or should this use the "opaque" spec
> > language? Well, I guess pod() makes the most sense for most of its uses.
> 
> I will rename to opaque to match the spec. It's not precisely the same as
> POD in C++
> (i.e., in C++ a Foo* is "POD"). 

opaque/transparent is actually one level of abstraction removed from what you're using pod() for in most places, but I guess that's better than adding a new incompatible term. (You can early-exit a visitReferences if it's transparent, but only because having references makes you opaque. You could imagine some other future rule that would make a type opaque even though it doesn't contain any references. But for now, it's totally fine the way it is.)

> > ...except I probably have the wrong terminology. Also, can you comment why
> > the js::Class is shared? (Especially given that it previously wasn't.)
> 
> I changed to a shared js::Class just because it made the code simpler (less
> template magic). There is no real performance implication, I don't think,
> particularly since I plan to optimize calls to these constructors in ion
> code.

Right, but the point is that a reader is likely to notice you're representing multiple things with the same Class, so it'd be nice to have a quickie comment saying why. A short form of the comment you already have above the class_ definition: "they have a reserved
 * slot pointing to a TypeRepresentation object, which is used to
 * distinguish which scalar type object this actually is."

So something like "the TypeRepresentation gives the specific scalar type".

> > Where is the JSJitInfo thing commented? (Just the 1-line, basic idea.)
> 
> I'm not sure.

From what I can tell, it isn't. You can kind of figure it out from the declaration in jsfriendapi.h (!). Ugh. I could make a guess at a high-level description, but never mind, not your problem.

> > @@ +80,5 @@
> > > +// These constants are for use exclusively in JS code. In C++ code,
> > > +// prefer ReferenceTypeRepresentation::TYPE_ANY etc, since that allows
> > > +// you to write a switch which will receive a warning if you omit a
> > > +// case.
> > > +#define JS_REFERENCETYPEREPR_ANY        0
> > 
> > JS_REFERENCETYPE_REPR_ANY, maybe?
> 
> I'm not sure why I adopted the convention of JS_XXXTYPEREPR_YYY vs
> JS_XXX_TYPE_REPR_YYY but I did use it consistently for the other similar
> constants, though not all across the file it seems. I think I wanted the `_`
> to denote the logical groupings vs the word groupings. But maybe it just
> makes it hard to read. I can clean this up.

Ok, whatever you think works best.

> > Also, would it be bad to make everything in this file be const ints in
> > namespace js instead?
> 
> The reason they are #define's is because they are used in TypedObject.js,
> which is processed by cpp, but which is still JavaScript.

Ah! Ok, fair enough.

> > That seems worrisome. Any ideas by now?
> 
> I found that CountHeap is not especially reliable. Stepping through the
> tracing code, it is doing the right thing. I have patch that modifies
> CountHeap to permit more reliable testing, I'll post the patch and have you
> review it, because it's nontrivial (though simple). Given that modified
> patch, the test passes (I'll attach the updated test too).

I'll look at that now.
Comment on attachment 832305 [details] [diff] [review]
Bug898359-part2.diff

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

This seems ok, though it seems like anywhere you'd use it, you should consider using findReferences instead.

Hm... not sure why findReferences is only in shell/js.cpp instead of TestingFunctions.cpp.
Attachment #832305 - Flags: review?(sphink) → review+
Comment on attachment 832307 [details] [diff] [review]
Bug898359-part3.diff

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

::: js/src/tests/ecma_6/TypedObject/referencetypetrace.js
@@ +56,5 @@
> +  // just use a constant string here, it's always reachable via the
> +  // atoms table. Same is true of "Hello" + "1" (an earlier
> +  // attempt) due to parser constant folding. So we have to make a
> +  // rabbit that's not constant foldable. But don't just use
> +  // Math.random(), since small integers are atoms already.

:-)
Attachment #832307 - Flags: review?(sphink) → review+
Try run #1: https://tbpl.mozilla.org/?tree=Try&rev=5bb62ba914cf
Try run #2: https://tbpl.mozilla.org/?tree=Try&rev=d49d303a862a

I resolved a problem in the 1st try run concerning jsreftests,
where apparently the TestingFunctions.cpp are not available,
by skipping the test when CountHeap is not found. I am not
overly pleased by this, seems easy to degenerate into the
code not being tested at all, but I don't know a better solution.

Try run #2 contains some miscellaneous failures that I am
fairly confident are unrelated to this patch, since it is
fairly focused on features that are not yet in widespread use.
Moreover, none of those errors occurred in try run #1.
https://hg.mozilla.org/mozilla-central/rev/5b797c0177d3
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Assignee: general → nmatsakis
Depends on: 945241
You need to log in before you can comment on or make changes to this bug.