Closed Bug 797123 Opened 7 years ago Closed 7 years ago

Fix JS_ALWAYS_TRUE(fallible ToNumber) in jstypedarray.cpp

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(1 file)

Apparently, it was once believed that ToNumber is fallible when given a primitive value. It has 2 or 3 different ways of failing on OOM. For example, if you're onverting a string containing a double, you might blow out memory constructing the dtoa cache.
Blocks: 797128
Comment on attachment 667157 [details] [diff] [review]
Fix JS_ALWAYS_TRUE(fallible ToNumber) in jstypedarray.cpp

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

Yeah, I don't know what I was thinking, thinking ToNumber(primitive) was infallible.

::: js/src/jstypedarray.cpp
@@ +2077,5 @@
>  
>              const Value *src = ar->getDenseArrayElements();
>              SkipRoot skipSrc(cx, &src);
> +            unsigned i;
> +            for (i = 0; i < len; ++i) {

for (uint32_t i ...

Minimizing scope is a good thing.  And array indexes are uint32_t, so that's the correct type to use here.

@@ +2087,4 @@
>          } else {
>              RootedValue v(cx);
>  
>              for (unsigned i = 0; i < len; ++i) {

This should be uint32_t as well, particularly as it flows into JSObject::getElement.
Attachment #667157 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Comment on attachment 667157 [details] [diff] [review]
> Fix JS_ALWAYS_TRUE(fallible ToNumber) in jstypedarray.cpp
> 
> Review of attachment 667157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yeah, I don't know what I was thinking, thinking ToNumber(primitive) was
> infallible.
> 
> ::: js/src/jstypedarray.cpp
> @@ +2077,5 @@
> >  
> >              const Value *src = ar->getDenseArrayElements();
> >              SkipRoot skipSrc(cx, &src);
> > +            unsigned i;
> > +            for (i = 0; i < len; ++i) {
> 
> for (uint32_t i ...
> 
> Minimizing scope is a good thing.  And array indexes are uint32_t, so that's
> the correct type to use here.

Oh, sorry. The scope weirdness is a leftover from when this used to be something like:

uint32_t startGeneration = runtime->gcCounter;
unsigned i = 0;
while (i < len && runtime->gcCounter == startGeneration) {
  ...
  i++;
}
while (i < len) {
  ...slow version of loop body that re-fetched the pointers every iteration...
  i++;
}

I was feeling very clever until I noticed that the 2nd loop body was guaranteed to never execute, because GC happens to imply failure and therefore early exit.
https://hg.mozilla.org/mozilla-central/rev/e51d8558ad64
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.