797123 - Fix JS_ALWAYS_TRUE(fallible ToNumber) in jstypedarray.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix JS_ALWAYS_TRUE(fallible ToNumber) in jstypedarray.cpp

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Steve Fink [:sfink] [:s:]]

(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.

Comment 4

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e51d8558ad64

try push: https://tbpl.mozilla.org/?tree=Try&rev=4762cd73c124

Comment 5

[Ed Morley [:emorley]]

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