1290422 - Remove JSErrorReport.messageArgs

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tooru Fujisawa [:arai]]

Separated from bug 1283710.

> JSErrorReport.messageArgs is used only in ExpandErrorArgumentsVA, and in
> most case JSErrorReport.messageArgs is created or assigned there.  Also, if
> it's created from va_list, I think it points to invalid address after
> leaving the frame.
> 
> Anyway, we don't need to store such information in JSErrorReport struct

Already have a patch in bug 1283710, but it would be better making it RAII class,
and it's necessary before adding JS_ReportError*{Latin1,UTF8} variants.
So I'd like to fix it separated, before others.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Remove JSErrorReport.messageArgs. r=jwalden

Patch from bug 1283710.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Add AutoMessageArgs RAII class to automatically free allocated args buffer.

And changed it to RAII class.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8775926 [details] [diff] [review]
Part 2: Add AutoMessageArgs RAII class to automatically free allocated args buffer.

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

::: js/src/jscntxt.cpp
@@ +464,4 @@
>  {
> +    const char16_t** args_;
> +    size_t totalLength_;
> +    size_t lengths_[JS::MaxNumErrorArguments]; /* only {0} thru {9} supported */

mozilla::Array<size_t, JS::MaxNumErrorArguments>;

@@ +516,5 @@
> +
> +    /*
> +     * Gather the arguments into an array, and accumulate their sizes. We
> +     * allocate 1 more than necessary and null it out to act as the caboose
> +     * when we free the pointers later.

s/caboose/sentinel value/, that's the more common name for this.  https://en.wikipedia.org/wiki/Sentinel_value

@@ +588,5 @@
>      }
>  
>      if (efs) {
> +        uint16_t argCount;
> +        AutoMessageArgs args;

Could this be declared inside the |argCount > 0| block, for greater specificity?  Looks like it, but I didn't track the pointer borrowing closely enough to be certain it'd be okay.

@@ +596,1 @@
>          argCount = efs->argCount;

Declare argCount here, i.e. as late as possible.

Comment 4

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cb1f015845f5b5e3aad0154add1929fad0a883
Bug 1290422 - Part 1: Remove JSErrorReport.messageArgs. r=jwalden

https://hg.mozilla.org/integration/mozilla-inbound/rev/c39c24ce6b51f17740cb827ca3df77b66e43a7a7
Bug 1290422 - Part 2: Add AutoMessageArgs RAII class to automatically free allocated args buffer. r=jwalden

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b4cb1f015845
https://hg.mozilla.org/mozilla-central/rev/c39c24ce6b51