Closed Bug 1290422 Opened 3 years ago Closed 3 years ago

Remove JSErrorReport.messageArgs

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(2 files)

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.
And changed it to RAII class.
Attachment #8775926 - Flags: review?(jwalden+bmo)
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.
Attachment #8775926 - Flags: review?(jwalden+bmo) → review+
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
https://hg.mozilla.org/mozilla-central/rev/b4cb1f015845
https://hg.mozilla.org/mozilla-central/rev/c39c24ce6b51
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.