510360 - early return in json.cpp might skip necessary postamble

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

In the big loop in the function 'JO' in json.cpp, it appears that most of the code funnels through a postamble after the loop that closes an iterator.  However, there are a few early returns in the loop that skip this close.

I'm not well versed in json or iterators, so perhaps there is a reason these early exits are fine.  If the reason isn't obvious, perhaps it could be added as a comment.

Comment 1

[Brendan Eich [:brendan]]

The GC will take care of the iterator, no js_CloseIterator required. It helps to promptly free memory, so worth doing, but control flow is a bit fussy already in JO. To make it fussier but consistent, all those early return JS_FALSE; statements should be breaks after if (!ok) tests, where ok = the condition after the ! in the existing condition, e.g.

            ok = scx->replacer->getProperty(cx, key, &newKey);
            if (!ok)
                break;

/be

Comment 2

[Brendan Eich [:brendan]]

Alternatively, for simpler code, get rid of the ok setting and if (!ok) break; jazz, and the if (!ok) return JS_FALSE; after the conditional js_CloseIterator, and just return early on error.

/be

Comment 3

[Luke Wagner [:luke]]

Attachment: uniform control flow

This patch normalizes the control flow so the iterators are always closed.  Shorter code too.

Comment 4

[Murali Nandigama [:murali]]

You have a patch on this bug that is flagged for 'review?' and not assigned to any reviewer. If you want the patch to be reviewed please assign a reviewer. Thanks

Comment 5

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/b03e2edfedb6

Comment 6

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/b03e2edfedb6

Comment 7

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1a1f1085a4bd