From 1565fca25081d8ede05a6e917f4924c92becfac6 Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Wed, 12 Apr 2017 14:13:21 -0700 Subject: [PATCH] Bug 1348134 - Pin chars returned from ScriptSource as an analog to Rooted. (r=jonco) --- js/src/jsfun.cpp | 7 ++-- js/src/jsgc.cpp | 27 ++++++++++----- js/src/jsscript.cpp | 67 ++++++++++++++++++++++++++++++------- js/src/jsscript.h | 49 ++++++++++++++++++++++++--- js/src/vm/HelperThreads.cpp | 2 +- 5 files changed, 122 insertions(+), 30 deletions(-) diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp index 2f6f09c22b60..da8c1fc000ff 100644 --- a/js/src/jsfun.cpp +++ b/js/src/jsfun.cpp @@ -1495,11 +1495,12 @@ JSFunction::createScriptForLazilyInterpretedFunction(JSContext* cx, HandleFuncti // Parse and compile the script from source. size_t lazyLength = lazy->end() - lazy->begin(); UncompressedSourceCache::AutoHoldEntry holder; - const char16_t* chars = lazy->scriptSource()->chars(cx, holder, lazy->begin(), lazyLength); - if (!chars) + ScriptSource::PinnedChars chars(cx, lazy->scriptSource(), holder, + lazy->begin(), lazyLength); + if (!chars.get()) return false; - if (!frontend::CompileLazyFunction(cx, lazy, chars, lazyLength)) { + if (!frontend::CompileLazyFunction(cx, lazy, chars.get(), lazyLength)) { // The frontend may have linked the function and the non-lazy // script together during bytecode compilation. Reset it now on // error. diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp index 1b77f725e325..9a064cdea721 100644 --- a/js/src/jsgc.cpp +++ b/js/src/jsgc.cpp @@ -4990,7 +4990,7 @@ MAKE_GC_SWEEP_TASK(SweepInitialShapesTask); MAKE_GC_SWEEP_TASK(SweepObjectGroupsTask); MAKE_GC_SWEEP_TASK(SweepRegExpsTask); MAKE_GC_SWEEP_TASK(SweepMiscTask); -MAKE_GC_SWEEP_TASK(AttachCompressedSourcesTask); +MAKE_GC_SWEEP_TASK(SweepCompressionTasksTask); #undef MAKE_GC_SWEEP_TASK /* virtual */ void @@ -5046,17 +5046,28 @@ SweepMiscTask::run() } /* virtual */ void -AttachCompressedSourcesTask::run() +SweepCompressionTasksTask::run() { AutoLockHelperThreadState lock; - GlobalHelperThreadState::SourceCompressionTaskVector& finished = - HelperThreadState().compressionFinishedList(lock); + + // Attach finished compression tasks. + auto& finished = HelperThreadState().compressionFinishedList(lock); for (size_t i = 0; i < finished.length(); i++) { SourceCompressionTask* task = finished[i]; if (task->runtimeMatches(runtime())) { + HelperThreadState().remove(finished, &i); task->complete(); js_delete(task); - HelperThreadState().remove(finished, &i); + } + } + + // Sweep pending tasks that are holding onto should-be-dead ScriptSources. + auto& pending = HelperThreadState().compressionPendingList(lock); + for (size_t i = 0; i < pending.length(); i++) { + SourceCompressionTask* task = pending[i]; + if (task->shouldCancel()) { + HelperThreadState().remove(pending, &i); + js_delete(task); } } } @@ -5139,7 +5150,7 @@ GCRuntime::beginSweepingSweepGroup(AutoLockForExclusiveAccess& lock) SweepObjectGroupsTask sweepObjectGroupsTask(rt); SweepRegExpsTask sweepRegExpsTask(rt); SweepMiscTask sweepMiscTask(rt); - AttachCompressedSourcesTask attachCompressedSourcesTask(rt); + SweepCompressionTasksTask sweepCompressionTasksTask(rt); WeakCacheTaskVector sweepCacheTasks = PrepareWeakCacheTasks(rt); for (GCSweepGroupIter zone(rt); !zone.done(); zone.next()) { @@ -5188,7 +5199,7 @@ GCRuntime::beginSweepingSweepGroup(AutoLockForExclusiveAccess& lock) startTask(sweepObjectGroupsTask, gcstats::PHASE_SWEEP_TYPE_OBJECT, helperLock); startTask(sweepRegExpsTask, gcstats::PHASE_SWEEP_REGEXP, helperLock); startTask(sweepMiscTask, gcstats::PHASE_SWEEP_MISC, helperLock); - startTask(attachCompressedSourcesTask, gcstats::PHASE_SWEEP_MISC, helperLock); + startTask(sweepCompressionTasksTask, gcstats::PHASE_SWEEP_MISC, helperLock); for (auto& task : sweepCacheTasks) startTask(task, gcstats::PHASE_SWEEP_MISC, helperLock); } @@ -5270,7 +5281,7 @@ GCRuntime::beginSweepingSweepGroup(AutoLockForExclusiveAccess& lock) joinTask(sweepObjectGroupsTask, gcstats::PHASE_SWEEP_TYPE_OBJECT, helperLock); joinTask(sweepRegExpsTask, gcstats::PHASE_SWEEP_REGEXP, helperLock); joinTask(sweepMiscTask, gcstats::PHASE_SWEEP_MISC, helperLock); - joinTask(attachCompressedSourcesTask, gcstats::PHASE_SWEEP_MISC, helperLock); + joinTask(sweepCompressionTasksTask, gcstats::PHASE_SWEEP_MISC, helperLock); for (auto& task : sweepCacheTasks) joinTask(task, gcstats::PHASE_SWEEP_MISC, helperLock); } diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp index 7229b24af6e2..919f4897d400 100644 --- a/js/src/jsscript.cpp +++ b/js/src/jsscript.cpp @@ -1624,6 +1624,45 @@ ScriptSource::chunkChars(JSContext* cx, UncompressedSourceCache::AutoHoldEntry& return ret; } +ScriptSource::PinnedChars::PinnedChars(JSContext* cx, ScriptSource* source, + UncompressedSourceCache::AutoHoldEntry& holder, + size_t begin, size_t len) + : source_(source) +{ + chars_ = source->chars(cx, holder, begin, len); + if (chars_) { + stack_ = &source->pinnedCharsStack_; + prev_ = *stack_; + *stack_ = this; + } +} + +ScriptSource::PinnedChars::~PinnedChars() +{ + if (chars_) { + MOZ_ASSERT(*stack_ == this); + *stack_ = prev_; + if (!prev_) + source_->movePendingCompressedSource(); + } +} + +void +ScriptSource::movePendingCompressedSource() +{ + if (!pendingCompressed_) + return; + + MOZ_ASSERT(data.is() || data.is()); + MOZ_ASSERT_IF(data.is(), + data.as().string.length() == + pendingCompressed_->uncompressedLength); + + data = SourceType(Compressed(mozilla::Move(pendingCompressed_->raw), + pendingCompressed_->uncompressedLength)); + pendingCompressed_ = mozilla::Nothing(); +} + const char16_t* ScriptSource::chars(JSContext* cx, UncompressedSourceCache::AutoHoldEntry& holder, size_t begin, size_t len) @@ -1711,10 +1750,10 @@ ScriptSource::substring(JSContext* cx, size_t start, size_t stop) MOZ_ASSERT(start <= stop); size_t len = stop - start; UncompressedSourceCache::AutoHoldEntry holder; - const char16_t* chars = this->chars(cx, holder, start, len); - if (!chars) + PinnedChars chars(cx, this, holder, start, len); + if (!chars.get()) return nullptr; - return NewStringCopyN(cx, chars, len); + return NewStringCopyN(cx, chars.get(), len); } JSFlatString* @@ -1723,10 +1762,10 @@ ScriptSource::substringDontDeflate(JSContext* cx, size_t start, size_t stop) MOZ_ASSERT(start <= stop); size_t len = stop - start; UncompressedSourceCache::AutoHoldEntry holder; - const char16_t* chars = this->chars(cx, holder, start, len); - if (!chars) + PinnedChars chars(cx, this, holder, start, len); + if (!chars.get()) return nullptr; - return NewStringCopyNDontDeflate(cx, chars, len); + return NewStringCopyNDontDeflate(cx, chars.get(), len); } JSFlatString* @@ -1784,8 +1823,10 @@ ScriptSource::setCompressedSource(SharedImmutableString&& raw, size_t uncompress MOZ_ASSERT(data.is() || data.is()); MOZ_ASSERT_IF(data.is(), data.as().string.length() == uncompressedLength); - - data = SourceType(Compressed(mozilla::Move(raw), uncompressedLength)); + if (pinnedCharsStack_) + pendingCompressed_ = mozilla::Some(Compressed(mozilla::Move(raw), uncompressedLength)); + else + data = SourceType(Compressed(mozilla::Move(raw), uncompressedLength)); } bool @@ -4347,16 +4388,16 @@ LazyScriptHashPolicy::match(JSScript* script, const Lookup& lookup) size_t scriptBegin = script->sourceStart(); size_t length = script->sourceEnd() - scriptBegin; - const char16_t* scriptChars = script->scriptSource()->chars(cx, holder, scriptBegin, length); - if (!scriptChars) + ScriptSource::PinnedChars scriptChars(cx, script->scriptSource(), holder, scriptBegin, length); + if (!scriptChars.get()) return false; MOZ_ASSERT(scriptBegin == lazy->begin()); - const char16_t* lazyChars = lazy->scriptSource()->chars(cx, holder, scriptBegin, length); - if (!lazyChars) + ScriptSource::PinnedChars lazyChars(cx, lazy->scriptSource(), holder, scriptBegin, length); + if (!lazyChars.get()) return false; - return !memcmp(scriptChars, lazyChars, length); + return !memcmp(scriptChars.get(), lazyChars.get(), length); } void diff --git a/js/src/jsscript.h b/js/src/jsscript.h index a7c8d5c98866..342962ee5d08 100644 --- a/js/src/jsscript.h +++ b/js/src/jsscript.h @@ -357,6 +357,33 @@ class ScriptSource { friend class SourceCompressionTask; + public: + // Any users that wish to manipulate the char buffer of the ScriptSource + // needs to do so via PinnedChars for GC safety. A GC may compress + // ScriptSources. If the source were initially uncompressed, then any raw + // pointers to the char buffer would now point to the freed, uncompressed + // chars. This is analogous to Rooted. + class PinnedChars + { + PinnedChars** stack_; + PinnedChars* prev_; + + ScriptSource* source_; + const char16_t* chars_; + + public: + PinnedChars(JSContext* cx, ScriptSource* source, + UncompressedSourceCache::AutoHoldEntry& holder, + size_t begin, size_t len); + + ~PinnedChars(); + + const char16_t* get() const { + return chars_; + } + }; + + private: uint32_t refs; // Note: while ScriptSources may be compressed off thread, they are only @@ -390,6 +417,12 @@ class ScriptSource using SourceType = mozilla::Variant; SourceType data; + // If the GC attempts to call setCompressedSource with PinnedChars + // present, the first PinnedChars (that is, bottom of the stack) will set + // the compressed chars upon destruction. + PinnedChars* pinnedCharsStack_; + mozilla::Maybe pendingCompressed_; + // The filename of this script. UniqueChars filename_; @@ -454,10 +487,21 @@ class ScriptSource const char16_t* chunkChars(JSContext* cx, UncompressedSourceCache::AutoHoldEntry& holder, size_t chunk); + // Return a string containing the chars starting at |begin| and ending at + // |begin + len|. + // + // Warning: this is *not* GC-safe! Any chars to be handed out should use + // PinnedChars. See comment below. + const char16_t* chars(JSContext* cx, UncompressedSourceCache::AutoHoldEntry& asp, + size_t begin, size_t len); + + void movePendingCompressedSource(); + public: explicit ScriptSource() : refs(0), data(SourceType(Missing())), + pinnedCharsStack_(nullptr), filename_(nullptr), displayURL_(nullptr), sourceMapURL_(nullptr), @@ -513,11 +557,6 @@ class ScriptSource return data.match(LengthMatcher()); } - // Return a string containing the chars starting at |begin| and ending at - // |begin + len|. - const char16_t* chars(JSContext* cx, UncompressedSourceCache::AutoHoldEntry& asp, - size_t begin, size_t len); - JSFlatString* substring(JSContext* cx, size_t start, size_t stop); JSFlatString* substringDontDeflate(JSContext* cx, size_t start, size_t stop); diff --git a/js/src/vm/HelperThreads.cpp b/js/src/vm/HelperThreads.cpp index 8d258613c827..afcbaeca3e3b 100644 --- a/js/src/vm/HelperThreads.cpp +++ b/js/src/vm/HelperThreads.cpp @@ -1781,8 +1781,8 @@ ClearCompressionTaskList(T& list, JSRuntime* runtime) for (size_t i = 0; i < list.length(); i++) { SourceCompressionTask* task = list[i]; if (task->runtimeMatches(runtime)) { - js_delete(task); HelperThreadState().remove(list, &i); + js_delete(task); } } }