1464845 - Remove js_strdup and JS_strdup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[André Bargull [:anba]]

Because Waldo said it's gone: https://mozilla.logbot.info/jsapi/20180525#c14811233

Comment 1

[André Bargull [:anba]]

Attachment: bug1464845.patch

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.

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[André Bargull [:anba]]

Attachment: bug1464845.patch

Updated per review comments, carrying r+.

Comment 4

[André Bargull [:anba]]

Try: https://hg.mozilla.org/try/rev/66e0deb6926fb5dfdbfdadd82312caf3c190c956

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 6

[André Bargull [:anba]]

Attachment: bug1464845.patch

Rebased patch, carrying r+.

Comment 7

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59678f7799f8642b3c58a72e76cd7855e4f756cd

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2426ac9d78fe
Remove js_strdup and JS_strdup. r=Waldo

Comment 9

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/3aaea8874060
Backed out changeset 2426ac9d78fe for Merge conflicts with bug 1465060

Comment 10

[André Bargull [:anba]]

Attachment: bug1464845.patch

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. :-)

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eae894f2ddc062a7bc58b7b513bae9a9cebe95cc

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2c785effde
Remove js_strdup and JS_strdup. r=Waldo

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/fc2c785effde