Closed Bug 1464845 Opened Last year Closed Last year

Remove js_strdup and JS_strdup

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch bug1464845.patch (obsolete) — Splinter Review
js/src/builtin/TestingFunctions.cpp
- Replace JS_strdup() with DuplicateString() and then directly move the result to InlineFrameInfo.
- Drive-by fix: Use "js_free" instead of "free" in GetLcovInfo(...).

js/src/jsapi.cpp
- Replace JS_strdup() with DuplicateString() + release() in JS::OwningCompileOptions::setFile(...) and JS::OwningCompileOptions::setIntroducerFilename(...), because no better alternative is currently possible.
- Directly return UniqueChars from JS_GetDefaultLocale().

js/src/vm/BytecodeUtil.cpp
- Directly return UniqueChars from DecompileValueGenerator(...).
- Change js::DecompileArgument(...) to return UniqueChars similar to DecompileValueGenerator(...).

js/src/vm/CodeCoverage.{h,cpp}
- Change LCovSource::name_ to UniqueChars to avoid manual resource management and update the constructors accordingly.
- Directly pass UniqueChars to the LCovSource constructor in LCovRealm::lookupOrAdd().

js/src/vm/Debugger.{h,cpp}
- Change EvalOptions::filename_ to UniqueChars.

js/src/vm/JSScript.cpp
- Replace js_strdup() with the version of DuplicateString() which takes a context, so we don't need to report oom-errors manually.

js/src/vm/Runtime.cpp
- Also only replaced JS_strdup() with DuplicateString() + release().
- Drive-by change: Only reset the default locale when DuplicateString(...) returned non-null result.

js/src/vm/SelfHosting.cpp
- Update to use UniqueChars now that js::DecompileArgument(...) no longer returns char*.
- Drive-by change: Don't atomize the result.

js/src/vm/UbiNodeCensus.cpp
- Make UniqueCString an alias for UniqueChars and then replace js_strdup() with DuplicateString() in ByFilename::count(...).

js/src/vm/SharedImmutableStringsCache.h
- Directly use SpiderMonkey's typedefs for JS::UniqueChars and JS::UniqueTwoByteChars instead of reinventing them.

js/src/vm/TraceLogging.{h,cpp}
- Change TraceLoggerEventPayload constructor to use UniqueChars&&.
- Replace js_strdup() with DuplicateString() in TraceLoggerThreadState::getOrCreateEventPayload() and directly move the result to TraceLoggerEventPayload.
- Update TraceLoggerThreadState::getOrCreateEventPayload() to use JS_smprintf(), so we directly get a UniqueChars result and don't need to compute the char-buffer size.
- Drive-by fix: Change type for line/column information from size_t to uint32_t as a follow-up for bug 1464321.


js/src/shell/js.cpp
- Remove unused methods from AutoCStringVector and update the other methods to take UniqueChars instead of char*.
- And then replace js_strdup() with the version of DuplicateString() which takes a context to properly report oom-errors.
- Replace JS_strdup() with DuplicateString() in SetContextOptions(...), but this time take the contextless DuplicateString() version for consistency with oom-errors from JS_smprintf and because I'm not sure if the JSContext is actually already sufficiently instantiated at this point to report errors.
Attachment #8981178 - Flags: review?(jwalden+bmo)
Comment on attachment 8981178 [details] [diff] [review]
bug1464845.patch

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

::: js/src/builtin/TestingFunctions.cpp
@@ +2400,5 @@
>      }
>  
>      struct InlineFrameInfo
>      {
> +        InlineFrameInfo(const char* kind, UniqueChars&& label)

No &&.

::: js/src/shell/js.cpp
@@ +4987,3 @@
>          return argv_.back();
>      }
> +    void replaceBack(UniqueChars&& arg) {

Might make this |UniqueChar arg| since this takes unconditionally, and the Move(...) is forced either way.

::: js/src/vm/CodeCoverage.cpp
@@ +509,5 @@
>          return nullptr;
>      }
>  
>      // Allocate a new LCovSource for the current top-level.
> +    if (!sources_->append(mozilla::Move(LCovSource(&alloc_, mozilla::Move(source_name))))) {

Switch in emplaceBack here, to avoid needing to grunge up with mozilla::Move around the LCovSource.

::: js/src/vm/CodeCoverage.h
@@ +25,5 @@
>  
>  class LCovSource
>  {
>    public:
> +    LCovSource(LifoAlloc* alloc, JS::UniqueChars&& name);

Should take value, not rvalue ref.

::: js/src/vm/TraceLogging.cpp
@@ +427,5 @@
>  
>      uint32_t textId = nextTextId;
>  
> +    TraceLoggerEventPayload* payload = js_new<TraceLoggerEventPayload>(textId,
> +                                                                       mozilla::Move(str));

I'd make this auto*.

@@ +476,3 @@
>      uint32_t textId = nextTextId;
> +    TraceLoggerEventPayload* payload = js_new<TraceLoggerEventPayload>(textId,
> +                                                                       mozilla::Move(str));

auto*

::: js/src/vm/TraceLogging.h
@@ +185,5 @@
>      mozilla::Atomic<uint32_t> uses_;
>      mozilla::Atomic<uint32_t> pointerCount_;
>  
>    public:
> +    TraceLoggerEventPayload(uint32_t textId, UniqueChars&& string)

Pass by value, not rvalue reference.
Attachment #8981178 - Flags: review?(jwalden+bmo) → review+
Attached patch bug1464845.patch (obsolete) — Splinter Review
Updated per review comments, carrying r+.
Attachment #8981178 - Attachment is obsolete: true
Attachment #8982447 - Flags: review+
Needs rebasing.
Keywords: checkin-needed
Attached patch bug1464845.patch (obsolete) — Splinter Review
Rebased patch, carrying r+.
Attachment #8982447 - Attachment is obsolete: true
Attachment #8982997 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2426ac9d78fe
Remove js_strdup and JS_strdup. r=Waldo
Keywords: checkin-needed
Backout by dluca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/3aaea8874060
Backed out changeset 2426ac9d78fe for Merge conflicts with bug 1465060
Attached patch bug1464845.patchSplinter Review
Rebased, carrying r+.

Now even with the proper emplaceBack(...) change as suggested in comment #2 which got lost on the way between https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e0deb6926fb5dfdbfdadd82312caf3c190c956 and https://treeherder.mozilla.org/#/jobs?repo=try&revision=027bb1d909bd48ac73f19b37952396bd136ea8f9. :-)
Attachment #8982997 - Attachment is obsolete: true
Attachment #8983208 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2c785effde
Remove js_strdup and JS_strdup. r=Waldo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fc2c785effde
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.