Closed
Bug 1290422
Opened 8 years ago
Closed 8 years ago
Remove JSErrorReport.messageArgs
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: arai, Assigned: arai)
References
Details
Attachments
(2 files)
(deleted),
patch
|
arai
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•8 years ago
|
||
And changed it to RAII class.
Attachment #8775926 -
Flags: review?(jwalden+bmo)
Comment 3•8 years ago
|
||
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+
Assignee | ||
Comment 4•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4cb1f015845
https://hg.mozilla.org/mozilla-central/rev/c39c24ce6b51
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•