972319 - New API JS::AutoSaveExceptionState, replacement for JS_SaveExceptionState

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

JS_SaveExceptionState is crazy. If nothing else, it does a tiny malloc which could fail. LOL.

It has a handful of uses and should probably have more. A class-based API would be pretty much tolerable. Patches coming.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: bug-972319-part-1-AutoSaveExceptionState-v1.patch

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: bug-972319-part-2-xpconnect-v1.patch

With this, the only remaining user of JS_SaveExceptionState is JSD1.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8377655 [details] [diff] [review]
bug-972319-part-1-AutoSaveExceptionState-v1.patch

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

::: js/src/jsapi.cpp
@@ +4590,4 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
>      assertSameCompartment(cx, obj);
> +    cx->clearPendingException();

It might be nice to have a \n before the clearPendingException() this to visually separate it from the preceding assertion block (which eyes are inclined to skip over).

::: js/src/jsapi.h
@@ +4636,2 @@
>  extern JS_PUBLIC_API(JSExceptionState *)
>  JS_SaveExceptionState(JSContext *cx);

Any reason not to just remove this now?  I doubt anyone using this will read the comment before whenever we do finally remove it.

Comment 4

[Jason Orendorff [:jorendorff]]

It can be removed when JSD is removed.

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8aed2eaed137
https://hg.mozilla.org/integration/mozilla-inbound/rev/aacc99e7c1e5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8aed2eaed137
https://hg.mozilla.org/mozilla-central/rev/aacc99e7c1e5