463320 - TM: Check for error return from Array_p_push1/pop on trace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jason Orendorff [:jorendorff]]

Traceable natives with errtype FAIL_JSVAL return JSVAL_ERROR_COOKIE to indicate that an error occurred, but currently we don't guard for that.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Is this really it?

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 351095 [details] [diff] [review]
Is this really it?

If push or pop fails in certain ways *after* modifying the array, then this has a bug.

OOM_EXIT causes control to return to the interpreter, which retries the instruction.

If retrying succeeds (e.g. if the traceable native failed because GC was needed), then the element was pushed or popped twice.

Offhand I don't see how that can happen.  But I don't like the fragility here.

r+ but let's rip this stuff out once we can call JSFastNatives on trace.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 351095 [details] [diff] [review]
Is this really it?

I take it back.  There may be a better reason we can't do this; redirecting the review to gal.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 351095 [details] [diff] [review]
Is this really it?

(In reply to comment #2)
> (From update of attachment 351095 [details] [diff] [review])
> If push or pop fails in certain ways *after* modifying the array, then this has
> a bug.
> 
> OOM_EXIT causes control to return to the interpreter, which retries the
> instruction.
> 
> If retrying succeeds (e.g. if the traceable native failed because GC was
> needed), then the element was pushed or popped twice.
> 
> Offhand I don't see how that can happen.  But I don't like the fragility here.

I do -- if we handled it correctly, push on an array with 2**32-2 elements would add properties and then throw when trying to set length to 2**32 (because ToUint32(2**32) != 2**32).

Frankly, I don't see how it's possible to differentiate between the two.  Maybe we could check whether such functions are pure or not?

Comment 5

[Jason Orendorff [:jorendorff]]

Oh, now I see what you were asking on IRC earlier.  Throwing an actual exception is no problem, because when we fall back into the interpreter, we check (cx->throwing) and we do not retry if it's set.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 351095 [details] [diff] [review]
Is this really it?

Ah, restoring flag then...

Comment 7

[Brendan Eich [:brendan]]

I mentioned a recent es-discuss thread on this topic, and Waldo recalled ES3.1 addressing the issue (or else that it should). If we can catch length overflow early before effects are committed, and track or get that into 3.1, we should.

/be

Comment 8

[Andreas Gal :gal]

Summary of discussion with waldo: push should return always int, and error flag if index very large (> signed int). pop should not ever error. If it does (sealed object) that it should check error first so we can re-execute safely.

Comment 9

[Andreas Gal :gal]

Note: we ALWAYS re-execute, even across branches.

Comment 10

[Andreas Gal :gal]

Comment on attachment 351095 [details] [diff] [review]
Is this really it?

Waldo will rewrite the patch as discussed above.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> Summary of discussion with waldo: push should return always int, and error flag
> if index very large (> signed int). pop should not ever error. If it does
> (sealed object) that it should check error first so we can re-execute safely.

Is comment 5 wrong?  I thought exceptions were no problem.

Comment 12

[Jeff Walden [:Waldo]]

I think I'm going to have to throw this back in the pool; I have other blockers I'm making much more progress on, and to be honest I don't actually remember the rationales that backed comment 8, so I'd need to address that first before making progress here anyway.  Next time that gets explained we need to make sure to put the explanation here so it's not forgotten. :-\

Comment 13

[Jason Orendorff [:jorendorff]]

This is P1, blocking, and unassigned.  I'll take it since I'm in that neighborhood right now...

Comment 14

[Jason Orendorff [:jorendorff]]

Note: There's a fix for this in the "part 2" patch in bug 462027.

Comment 15

[Jason Orendorff [:jorendorff]]

Fixed in tracemonkey when bug 462027 landed.

Comment 16

[Robert Sayre]

bug 462027 is fixed1.9.1, so this is too.