Closed
Bug 1211723
Opened 9 years ago
Closed 8 years ago
share ScriptSource between JSRuntimes via parentRuntime
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: luke, Assigned: fitzgen)
References
Details
(Whiteboard: [MemShrink:P1])
Attachments
(9 files, 35 obsolete files)
441.42 KB,
image/png
|
Details | |
130.59 KB,
image/png
|
Details | |
66.52 KB,
image/png
|
Details | |
417.97 KB,
application/x-gzip
|
Details | |
427.63 KB,
application/x-gzip
|
Details | |
65.00 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
30.90 KB,
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
2.67 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.65 KB,
patch
|
fitzgen
:
feedback+
|
Details | Diff | Splinter Review |
With multiple workers (esp. running threaded SharedArrayBuffer code), it's likely that scripts will be duplicated between workers. With asm.js code, the scripts can be quite large (80mb). There is already a JSRuntime::compressedSourceSet that shares ScriptSource within a JSRuntime, so the logical extension would be to share this field via JSRuntime::parentRuntime, protected by a lock and for the ScriptSource refct to be atomic. On additional requirement for this to really help asm.js is that the compressedSourceSet be generalized to include permanently-uncompressed sources (due to performance faults involving compression of huge scripts, scripts bigger than 5MiB [1] are left permanently decompressed). I think this is pretty simple. [1] http://hg.mozilla.org/mozilla-central/file/67adec79eb8a/js/src/jsscript.cpp#l1919
Comment 2•8 years ago
|
||
Bug 1244841 comment 7 is a great example of this: 64 MB of code duplicated across the main thread and 16 workers!
Updated•8 years ago
|
Whiteboard: [MemShrink]
Updated•8 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P1]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
TODO FITZGEN
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
This patch series is enough to get the jit-tests passing, but there are a few bugs. The filename_, displayURL_, and sourceMapURL_ members are all given out such that references exist longer than the SharedScriptSource data is exclusively locked, for example via JSScript::filename(). Meaning we have some data races that could lead to use-after-frees. I think we could do some kind of immutable-after-initialization thing for filename_ and displayURL_. However, we actually have setters for the sourceMapURL_ field exposed to chrome JS via Debugger.Source.prototype.sourceMapURL, so the immutable-after-init approach wouldn't work here. I think I took a slightly wrong approach by making all of ScriptSource and its filename/etc shared between runtimes. Instead, I think we should still have each ScriptSource belong to a runtime, and share only the source text content between runtimes. This would narrow the scope of the sharing to something more manageable and still give us the vast majority of benefits as the filename/etc are tiny compared to the source text contents.
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8726503 -
Attachment is obsolete: true
Attachment #8726504 -
Attachment is obsolete: true
Attachment #8726505 -
Attachment is obsolete: true
Attachment #8726506 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Comment 11•8 years ago
|
||
OK, so this time around we are sharing only the underlying source text and not any of the source's metadata, such as the filename. Passing most jit-tests, but there is one worrying failure related to compression/decompression and zlib, and a bunch of OOM-test failures related to the way I hacked up an infallible ExclusiveData constructor while biding time for when it really becomes infallible to create. Going to pause work here for a couple days as I help Till out with Debugger-related issues for Promises and take a look at some other crashers. I'm going to eventually flag both of you (:jimb, :terrence) for review, because this is a little bit sticky and I want to make sure we do it right ;)
Assignee | ||
Comment 12•8 years ago
|
||
TODO: * Fix failing tests (obviously) * Add an `UncompressedSourceSet` to the parent JSRuntime, which is similar to the `CompressedSourceSet` but for (you guessed it) sources we aren't compressing for whatever reason. * Add a `js::SharedSourceText::Create()` static method that checks the parent runtime's source sets to de-duplicate new sources, and use this where we are otherwise using js_new<SharedSourceText>() currently.
Assignee | ||
Comment 13•8 years ago
|
||
Attachment #8731885 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8728017 -
Attachment is obsolete: true
Attachment #8728018 -
Attachment is obsolete: true
Attachment #8728019 -
Attachment is obsolete: true
Attachment #8728020 -
Attachment is obsolete: true
Assignee | ||
Comment 14•8 years ago
|
||
Attachment #8731886 -
Flags: review?(terrence)
Attachment #8731886 -
Flags: review?(jimb)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8731887 -
Flags: review?(terrence)
Attachment #8731887 -
Flags: review?(jimb)
Assignee | ||
Comment 16•8 years ago
|
||
SharedSourceText is atomically refcounted and shared between JSRuntime threads. The actual chars are stored in SharedSourceText::SourceText, which is wrapped in an ExclusiveData to ensure that only one thread has access at a time. A SourceText is either Compressed or Uncompressed. This also adds an UncompressedSourceSet to the parent runtime. When creating new sources, we check this set first and return an extant source if we already have one for these chars. Compressing a source's text removes it from this set and adds it to the parent runtime's CompressedSourceSet which also performs deduplication. Bug 1211723 - Part 4: Create a set for deduplicating uncompressed sources on the parent runtime; r=terrence,jimb
Attachment #8731888 -
Flags: review?(terrence)
Attachment #8731888 -
Flags: review?(jimb)
Assignee | ||
Comment 17•8 years ago
|
||
This patch series shaves almost a full GB of memory usage off from the testcase in bug 1244841 comment 0.
Assignee | ||
Comment 18•8 years ago
|
||
Try push (there are likely still a few issues that haven't been uncovered just from jit tests locally yet): https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e2920807a00
Comment 19•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #17) > This patch series shaves almost a full GB of memory usage off from the > testcase in bug 1244841 comment 0. Very nice!
Comment 20•8 years ago
|
||
Comment on attachment 8731885 [details] [diff] [review] Part 0: Add asserts against re-entrancy to js::ExclusiveData<T> Review of attachment 8731885 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/threading/ExclusiveData.h @@ +95,5 @@ > ExclusiveData(const ExclusiveData&) = delete; > ExclusiveData& operator=(const ExclusiveData&) = delete; > > + void acquire() const { > + lock_.lock(); Maybe also assert !lockOwner_ here?
Attachment #8731885 -
Flags: review?(terrence) → review+
Comment 21•8 years ago
|
||
Comment on attachment 8731886 [details] [diff] [review] Part 1: Share CompressedSourceSet between JSRuntimes on the parentRuntime Review of attachment 8731886 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Runtime.cpp @@ +547,5 @@ > > rtSizes->uncompressedSourceCache += uncompressedSourceCache.sizeOfExcludingThis(mallocSizeOf); > > + { > + auto set = compressedSourceSet().lock(); This way, the parent and children all count the source set as their own. Wouldn't it make more sense to attribute the source set to the parent alone?
Attachment #8731886 -
Flags: review?(jimb) → review+
Comment 22•8 years ago
|
||
Comment on attachment 8731887 [details] [diff] [review] Part 2: Share UncompressedSourceCache between JSRuntimes on the parentRuntime Review of attachment 8731887 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Runtime.cpp @@ +553,5 @@ > > + { > + auto cache = uncompressedSourceCache().lock(); > + rtSizes->uncompressedSourceCache += cache->sizeOfExcludingThis(mallocSizeOf); > + } Similarly to the other patch: perhaps we should attribute this only to the parent.
Attachment #8731887 -
Flags: review?(jimb) → review+
Comment 23•8 years ago
|
||
Comment on attachment 8731888 [details] [diff] [review] Part 3: Create SharedSourceText to share ScriptSource's underlying chars between runtimes Review of attachment 8731888 [details] [diff] [review]: ----------------------------------------------------------------- The comment on the patch ought to be in the source somewhere, and in much more detail. The comments in jsscript.h are inadequate; there needs to be a general exposition on where the shared data lives, how it moves in and out of the various caches, who can point to it when, and so on; it's not okay to make people infer these things by following how the members are used. When there's enough of an explanation that I can orient myself and check the patch against the explanation, then I'll re-review. It had better be easy for me, because once it's landed, other people encountering this code will not be as interested in it as I am, as the reviewer. ::: js/src/jsscript.h @@ +559,5 @@ > +struct UncompressedSourceLookup; > + > +/** > + * The `SharedSourceText` class enables sharing source text contents between > + * JSRuntime threads. `SharedSourceText` is atomically reference counted, and "between JSRuntime threads" makes me think of threads belonging to a particular JSRuntime. What I think you mean is "enables sharing source text contents between multiple threads using multiple JSRuntimes", right? @@ +582,5 @@ > + UniquePtr<char16_t[], JS::FreePolicy> chars; > + // Length of the chars. Note that chars may contain null bytes. > + size_t length; > + // True iff this owner's SharedSourceText is in the parent runtime's > + // UncompressedSourceSet. I don't really understand this comment. Which owner? Do you mean: True if the owner of the SourceText to which this belongs is in the parent runtime's ... @@ +601,5 @@ > + : chars(mozilla::Move(rhs.chars)) > + , length(rhs.length) > + , inUncompressedSourceSet(rhs.inUncompressedSourceSet) > + , hash(mozilla::Move(rhs.hash)) > + { } Should this have a self-move check? @@ +618,5 @@ > + HashNumber hash; > + // Length of the *un*compressed data. > + size_t length; > + // True iff this owner's SharedSourceText is in the parent runtime's > + // CompressedSourceSet. Same comment as above. @@ +648,5 @@ > + * The source text is either compressed or uncompressed. > + */ > + class SourceText : public mozilla::Variant<Compressed, Uncompressed> > + { > + friend class SharedSourceText; This friend declaration isn't necessary, since we're a member class of SharedSourceText. @@ +749,5 @@ > + // incremented for the caller. > + // > + // `F intoOwnedChars` should return a `UniquePtr<char16_t[], JS::FreePolicy>` > + // version of chars. This allows callers to elide a copy of the chars when > + // possible. It should handle reporting exceptions on the context itself. You should provide the signature for intoOwnedChars, and expand on this 'elide copy' thing.
Attachment #8731888 -
Flags: review?(jimb) → review-
Comment 24•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #23) > When there's enough of an explanation that I can orient myself and check the > patch against the explanation, then I'll re-review. It had better be easy > for me, because once it's landed, other people encountering this code will > not be as interested in it as I am, as the reviewer. My tone here was not great; I'm sorry about that. I was in a hurry to get home and didn't re-read. What I should have said was: As the reviewer, I'm ready to invest energy in understanding how the patch works, so I can have confidence it's correct before I approve it. But subsequent readers will have some other purpose in mind when they encounter this code, so if it's hard for me to understand, it's *definitely* going to be hard for them.
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8731886 [details] [diff] [review] Part 1: Share CompressedSourceSet between JSRuntimes on the parentRuntime These patches aren't atomic, like I thought they were. Canceling review for now, squashing the patches, fixing failures in that try push, and then I will reflag.
Attachment #8731886 -
Flags: review?(terrence)
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8731887 [details] [diff] [review] Part 2: Share UncompressedSourceCache between JSRuntimes on the parentRuntime Ditto.
Attachment #8731887 -
Flags: review?(terrence)
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8731888 [details] [diff] [review] Part 3: Create SharedSourceText to share ScriptSource's underlying chars between runtimes You know the deal
Attachment #8731888 -
Flags: review?(terrence)
Assignee | ||
Comment 28•8 years ago
|
||
Ok, Jim and I just talked and discovered a potential deadlock condition based on the order we lock the sets and individual source texts in different places. Need some more refactoring.
Assignee | ||
Comment 29•8 years ago
|
||
Attachment #8733134 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8731885 -
Attachment is obsolete: true
Attachment #8731886 -
Attachment is obsolete: true
Attachment #8731887 -
Attachment is obsolete: true
Attachment #8731888 -
Attachment is obsolete: true
Assignee | ||
Comment 30•8 years ago
|
||
Lots of bug fixes! Assertions against dead locks! Woo! Let's see if this try push does any better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca393f95c122
Assignee | ||
Comment 31•8 years ago
|
||
Add missing header includes, add missing namespace qualifiers, and fix bad include orders. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f709fca7517
Assignee | ||
Updated•8 years ago
|
Attachment #8733135 -
Attachment is obsolete: true
Assignee | ||
Comment 32•8 years ago
|
||
Sigh... I forgot one header include. I am clearly getting a different unified grouping than the try builds... Yet Another Try Push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=025fc6d02469
Assignee | ||
Updated•8 years ago
|
Attachment #8733145 -
Attachment is obsolete: true
Assignee | ||
Comment 33•8 years ago
|
||
Ok, I finally set FILES_PER_UNIFIED_FILE=1 locally and worked out all the kinks. Should have done that way sooner... https://treeherder.mozilla.org/#/jobs?repo=try&revision=e280df892733
Assignee | ||
Updated•8 years ago
|
Attachment #8733148 -
Attachment is obsolete: true
Assignee | ||
Comment 34•8 years ago
|
||
Ok, this is getting much closer. Jit tests passing, most mochitests and xpcshell tests passing! A few assertion failures, but they all seem to be pretty concrete and point right to where the bugs are. A lot of build failures in my attempt to stub out nspr threads for DEBUG only assertions. I think I should just include jslock.h or whatever js thing interfaces with nspr until we have js::Thread::current().
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8733134 [details] [diff] [review] Part 0: Add asserts against re-entrancy to js::ExclusiveData<T> This is done more better-er and more general-er in bug 1258847.
Attachment #8733134 -
Attachment is obsolete: true
Assignee | ||
Comment 36•8 years ago
|
||
The failing mochitests on OSX are pthread_mutex_lock returning EINVAL which is either: * "The mutex was created with the protocol attribute having the value PTHREAD_PRIO_PROTECT and the calling thread's priority is higher than the mutex's current priority ceiling." * "The value specified by mutex does not refer to an initialized mutex object." We don't use PTHREAD_PRIO_PROTECT in js::Mutex, so I suspect it must be the latter. Still debugging. Haven't looked into the windows failures yet.
Assignee | ||
Comment 37•8 years ago
|
||
Ok, need to back up and simplify the design a bit. As it is now, this is trying to be too fancy, too fine grained w/ locking, and it makes destroying shared sources a huge pain because of inversion issues, and GC interaction, and the safety of the refcounting, etc. I keep fixing Just One More Thing and the architecture gets more and more convoluted. Even if we got it into a place where it was working correctly now, we wouldn't be able to solve any bugs introduced in the future. Going to back up and have a single lock protect a cache of shared and immutable strings. It will be totally orthogonal to ScriptSource and various source text related things. Then, after this dead simple cache of shared and immutable strings is implemented and solid, I will start making ScriptSource's text contents use it. The strings will be opaque to the cache, it will have no idea what they are for, if the data is compressed or not, etc. I think this design will be much simpler to implement and debug.
Assignee | ||
Comment 38•8 years ago
|
||
For posterity, here is my current patch that tries to be way too clever and fine grained with the locking.
Assignee | ||
Updated•8 years ago
|
Attachment #8733169 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8734195 -
Attachment is obsolete: true
Assignee | ||
Comment 39•8 years ago
|
||
Assignee | ||
Comment 40•8 years ago
|
||
Assignee | ||
Comment 41•8 years ago
|
||
Assignee | ||
Comment 42•8 years ago
|
||
Attachment #8734825 -
Flags: review?(jimb)
Assignee | ||
Comment 43•8 years ago
|
||
This adds a SharedImmutableStringsCache to every JSRuntime, but the accessor always returns the parent runtime's cache. The caches in child JSRuntime's are not wasting space, however, as initialization and allocation of the table happens lazily within SharedImmutableStringsCache.
Attachment #8734826 -
Flags: review?(jimb)
Assignee | ||
Comment 44•8 years ago
|
||
Attachment #8734827 -
Flags: review?(jimb)
Assignee | ||
Comment 45•8 years ago
|
||
Attachment #8734828 -
Flags: review?(jimb)
Assignee | ||
Comment 46•8 years ago
|
||
This commit removes `js::ScriptSource::Parent` and the `CompressedSourceSet`. They are unnecessary because source text is always shared via the parent runtime's `SharedImmutableStringsCache` now.
Attachment #8734829 -
Flags: review?(jimb)
Assignee | ||
Comment 47•8 years ago
|
||
Attachment #8734830 -
Flags: review?(jimb)
Assignee | ||
Comment 48•8 years ago
|
||
Attachment #8734831 -
Flags: review?(jimb)
Assignee | ||
Comment 49•8 years ago
|
||
This approach is *SO* much easier than the last one! Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8d10c9be94b
Assignee | ||
Comment 50•8 years ago
|
||
Comment on attachment 8734827 [details] [diff] [review] Part 2: Move source length from ScriptSource into the data variants Review of attachment 8734827 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.h @@ +638,5 @@ > > void* raw; > size_t nbytes; > HashNumber hash; > + size_t length; Note that this patch is mostly busy work, but makes the following parts a little bit more lucid.
Assignee | ||
Comment 51•8 years ago
|
||
Attachment #8734885 -
Flags: review?(jimb)
Assignee | ||
Updated•8 years ago
|
Attachment #8734825 -
Attachment is obsolete: true
Attachment #8734825 -
Flags: review?(jimb)
Assignee | ||
Comment 52•8 years ago
|
||
Attachment #8734886 -
Flags: review?(jimb)
Assignee | ||
Updated•8 years ago
|
Attachment #8734828 -
Attachment is obsolete: true
Attachment #8734828 -
Flags: review?(jimb)
Assignee | ||
Comment 53•8 years ago
|
||
Fixing up some warnings-as-errors fallout. New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c097296f2b2
Assignee | ||
Comment 54•8 years ago
|
||
Fix dumb bug introduced when fixing warnings-as-errors bugs.
Attachment #8734933 -
Flags: review?(jimb)
Assignee | ||
Updated•8 years ago
|
Attachment #8734885 -
Attachment is obsolete: true
Attachment #8734885 -
Flags: review?(jimb)
Assignee | ||
Comment 55•8 years ago
|
||
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaa9e50f8394
Comment 56•8 years ago
|
||
Pro-tip: about:memory is just text. You can copy and paste its contents :)
Assignee | ||
Comment 57•8 years ago
|
||
Heh, good call :)
Updated•8 years ago
|
Attachment #8734826 -
Flags: review?(jimb) → review+
Comment 58•8 years ago
|
||
Comment on attachment 8734933 [details] [diff] [review] Part 0: Create a cache for shared, immutable strings Review of attachment 8734933 [details] [diff] [review]: ----------------------------------------------------------------- Really great stuff. ::: js/src/jsapi-tests/testSharedImmutableStringsCache.cpp @@ +65,5 @@ > + CHECK(threads.reserve(NUM_THREADS)); > + > + for (auto i : mozilla::MakeRange(NUM_THREADS)) { > + auto cacheAndIndex = js_new<CacheAndIndex>(&cache, i); > + CHECK(cacheAndIndex); optional nit: You could probably add a PRThread* member to CacheAndIndex (I'd call it ThreadClosure) and then make `threads` a vector of those. ::: js/src/vm/SharedImmutableStringsCache.cpp @@ +35,5 @@ > +{ > + auto clone = cache_->getOrCreate(chars(), length(), [&]() { > + MOZ_CRASH("Should not need to create an owned version, as this string is already in " > + "the cache"); > + return nullptr; Isn't there an always-crash "unreachable" macro? ::: js/src/vm/SharedImmutableStringsCache.h @@ +10,5 @@ > +#include "mozilla/Maybe.h" > +#include "mozilla/UniquePtr.h" > + > +#include <cstring> > +#include <new> Could we put '// for placement new' here? (Vector.h does this, and I would have found it helpful.) @@ +34,5 @@ > +{ > + friend class SharedImmutableStringsCache; > + friend class SharedImmutableTwoByteString; > + > + SharedImmutableStringsCache* cache_; It would be helpful to comment, "Never nullptr in a live instance. May be nullptr if this instance has been moved from." @@ +38,5 @@ > + SharedImmutableStringsCache* cache_; > + const char* chars_; > + size_t length_; > +#ifdef DEBUG > + HashNumber hash_; Just checking: DebugOnly doesn't serve here because it's a field, not a local, so it can't be entirely eliminated, and instead occupies a byte even in non-DEBUG builds. *sigh* @@ +49,5 @@ > +#ifdef DEBUG > + , hash_(mozilla::HashString(chars, length)) > +#endif > + { > + MOZ_ASSERT(chars); Assert `cache` too? @@ +112,5 @@ > +class SharedImmutableTwoByteString > +{ > + friend class SharedImmutableStringsCache; > + > + SharedImmutableString string_; So the idea is, if a char and char16_t string just happen to be the same bytes, we'll just hand it out under two types? That's fine, but it would be worth spelling out, in a comment on this member. @@ +118,5 @@ > + explicit SharedImmutableTwoByteString(SharedImmutableString&& string) > + : string_(mozilla::Move(string)) > + { } > + > + SharedImmutableTwoByteString(SharedImmutableStringsCache* cache, const char* chars, size_t length) Perhaps I'll take this back when I see how it's used, but I was expecting this to take char16_t, and then cast it when initializing `string_`. @@ +119,5 @@ > + : string_(mozilla::Move(string)) > + { } > + > + SharedImmutableTwoByteString(SharedImmutableStringsCache* cache, const char* chars, size_t length) > + : string_(cache, chars, length) Assert that `length` is a multiple of sizeof(char16_t)? @@ +177,5 @@ > + > + /** > + * Get the canonical, shared, and de-duplicated version of the given `const > + * char*` string. In the case that such a string does not exist, > + * `intoOwnedChars` is called and the string is added to the cache. So great. No need for passive voice. "If such a string does not exist, call `intoOwnedChars` and add the string it returns to the cache." @@ +207,5 @@ > + OwnedChars ownedChars(intoOwnedChars()); > + if (!ownedChars) > + return mozilla::Nothing(); > + MOZ_ASSERT(ownedChars.get() == chars || > + memcmp(ownedChars.get(), chars, length) == 0); delightful @@ +273,5 @@ > + OwnedTwoByteChars ownedTwoByteChars(intoOwnedTwoByteChars()); > + if (!ownedTwoByteChars) > + return mozilla::Nothing(); > + MOZ_ASSERT(ownedTwoByteChars.get() == chars || > + memcmp(ownedTwoByteChars.get(), chars, length) == 0); length * sizeof(char16_t), right? @@ +297,5 @@ > + MOZ_WARN_UNUSED_RESULT mozilla::Maybe<SharedImmutableTwoByteString> > + getOrCreate(OwnedTwoByteChars&& chars, size_t length); > + > + /** > + * Do not take ownership of the given `chars`, return the canonical, shared "`chars`;" instead of "`chars`," or full stop? @@ +299,5 @@ > + > + /** > + * Do not take ownership of the given `chars`, return the canonical, shared > + * and de-duplicated version. If there is no extant shared version of > + * `chars`, then a copy will made. "will be made" @@ +394,5 @@ > + return memcmp(key->chars(), lookup.chars_, key->length()) == 0; > + } > + }; > + > + using Set = HashSet<StringBox::Ptr, Hasher, SystemAllocPolicy>; Could HashSet hold StringBoxes directly? StringBox holds a pointer to the chars, so they'll always be stable. SharedImmutableString re-hashes rather than pointing to a StringBox, so it doesn't need the StringBox to be stable.
Attachment #8734933 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 59•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #58) > Comment on attachment 8734933 [details] [diff] [review] > Part 0: Create a cache for shared, immutable strings > > Review of attachment 8734933 [details] [diff] [review]: > ----------------------------------------------------------------- > > Really great stuff. Thanks! > @@ +38,5 @@ > > + SharedImmutableStringsCache* cache_; > > + const char* chars_; > > + size_t length_; > > +#ifdef DEBUG > > + HashNumber hash_; > > Just checking: DebugOnly doesn't serve here because it's a field, not a > local, so it can't be entirely eliminated, and instead occupies a byte even > in non-DEBUG builds. *sigh* Yes, and also DebugOnly is a MOZ_STACK_CLASS these days. > @@ +394,5 @@ > > + return memcmp(key->chars(), lookup.chars_, key->length()) == 0; > > + } > > + }; > > + > > + using Set = HashSet<StringBox::Ptr, Hasher, SystemAllocPolicy>; > > Could HashSet hold StringBoxes directly? StringBox holds a pointer to the > chars, so they'll always be stable. SharedImmutableString re-hashes rather > than pointing to a StringBox, so it doesn't need the StringBox to be stable. Good catch!
Updated•8 years ago
|
Attachment #8734827 -
Flags: review?(jimb) → review+
Comment 60•8 years ago
|
||
Comment on attachment 8734886 [details] [diff] [review] Part 3: Make ScriptSource use the SharedImmutableStringsCache for its source text Review of attachment 8734886 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +1797,5 @@ > + mozilla::UniquePtr<char16_t[], JS::FreePolicy> ownedSource(src); > + auto& cache = cx->runtime()->sharedImmutableStrings(); > + auto deduped = cache.getOrCreate(mozilla::Move(ownedSource), length); > + if (!deduped) { > + ReportOutOfMemory(cx); We're introducing this boilerplate in several places, as we lift up the de-duplication to the higher level. It's never nice to see call sites being made more complicated. Would it be possible to give ScriptSource its own methods that take care of this? (Perhaps there's a later patch in the queue...) ::: js/src/jsstr.cpp @@ +4754,5 @@ > + if (!ret) > + return nullptr; > + PodCopy(ret.get(), s, n); > + ret[n] = 0; > + return ret; How is this legal? Isn't this an attempt to copy-assign a UniqueChars?
Attachment #8734886 -
Flags: review?(jimb) → review+
Comment 61•8 years ago
|
||
Comment on attachment 8734829 [details] [diff] [review] Part 4: Remove source de-duplication efforts that have been superseded Review of attachment 8734829 [details] [diff] [review]: ----------------------------------------------------------------- This is my favorite patch so far. ::: js/src/jsscript.h @@ +701,3 @@ > { > } > + ~ScriptSource() { } Isn't this the same as just omitting any explicit destructor? ::: js/src/vm/Runtime.h @@ -1286,5 @@ > js::UncompressedSourceCache uncompressedSourceCache; > js::EvalCache evalCache; > js::LazyScriptCache lazyScriptCache; > > - js::CompressedSourceSet compressedSourceSet; YESSSS
Attachment #8734829 -
Flags: review?(jimb) → review+
Comment 62•8 years ago
|
||
Comment on attachment 8734886 [details] [diff] [review] Part 3: Make ScriptSource use the SharedImmutableStringsCache for its source text Review of attachment 8734886 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ +2042,2 @@ > > + data = SourceType(Compressed(mozilla::Move(raw), hash, length)); I really like how this automatically takes care of dropping a reference from the compressed text.
Updated•8 years ago
|
Attachment #8734830 -
Flags: review?(jimb) → review+
Updated•8 years ago
|
Attachment #8734831 -
Flags: review?(jimb) → review+
Comment 63•8 years ago
|
||
Really nice. Makes it look easy. LOL
Comment 64•8 years ago
|
||
As a sanity check it would be great if you could do a DMD run prior to landing [1]. This will help identify any significantly large unreported chunks we may have missed as well as possible double-reporting. [1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 65•8 years ago
|
||
Flags: needinfo?(nfitzgerald)
Assignee | ||
Comment 66•8 years ago
|
||
Assignee | ||
Comment 67•8 years ago
|
||
Eric, I'm not 100% sure what I'm looking for in these dmd output files. I ran dmd.py on the output with the patch series applied. I don't have any double counting, and I'm not finding any unreported chunks with `SharedImmutableStringsCache`-related things on the allocation stack. Anything else I should look for?
Flags: needinfo?(erahm)
Comment 68•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #67) > Eric, I'm not 100% sure what I'm looking for in these dmd output files. I > ran dmd.py on the output with the patch series applied. I don't have any > double counting, and I'm not finding any unreported chunks with > `SharedImmutableStringsCache`-related things on the allocation stack. > Anything else I should look for? Yeah, high level it looks good. Thanks for taking time to measure.
Flags: needinfo?(erahm)
Assignee | ||
Comment 69•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #58) > @@ +35,5 @@ > > +{ > > + auto clone = cache_->getOrCreate(chars(), length(), [&]() { > > + MOZ_CRASH("Should not need to create an owned version, as this string is already in " > > + "the cache"); > > + return nullptr; > > Isn't there an always-crash "unreachable" macro? Unfortunately, there's only either * MOZ_ASSERT_UNREACHABLE: debug only assert, and still requires "default" return for release builds. * MOZ_MAKE_COMPILER_ASSUME_UNREACHABLE: makes the compiler consider the block unreachable and results in UB if it is reached. But what I want is more like MOZ_ASSERT_UNREACHABLE that is not debug only. I suppose I can create MOZ_RELEASE_ASSERT_UNREACHABLE...
Assignee | ||
Comment 70•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #60) > Comment on attachment 8734886 [details] [diff] [review] > Part 3: Make ScriptSource use the SharedImmutableStringsCache for its source > text > > Review of attachment 8734886 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/jsscript.cpp > @@ +1797,5 @@ > > + mozilla::UniquePtr<char16_t[], JS::FreePolicy> ownedSource(src); > > + auto& cache = cx->runtime()->sharedImmutableStrings(); > > + auto deduped = cache.getOrCreate(mozilla::Move(ownedSource), length); > > + if (!deduped) { > > + ReportOutOfMemory(cx); > > We're introducing this boilerplate in several places, as we lift up the > de-duplication to the higher level. It's never nice to see call sites being > made more complicated. Would it be possible to give ScriptSource its own > methods that take care of this? > > (Perhaps there's a later patch in the queue...) Will do this as a follow up. > > ::: js/src/jsstr.cpp > @@ +4754,5 @@ > > + if (!ret) > > + return nullptr; > > + PodCopy(ret.get(), s, n); > > + ret[n] = 0; > > + return ret; > > How is this legal? Isn't this an attempt to copy-assign a UniqueChars? Per spec, there are certain scenarios where copies/moves can be elided and most of them involve return values. This is one of them.
Assignee | ||
Comment 71•8 years ago
|
||
Addressed review comments and squashed the patches into a single commit.
Attachment #8736047 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8734826 -
Attachment is obsolete: true
Attachment #8734827 -
Attachment is obsolete: true
Attachment #8734829 -
Attachment is obsolete: true
Attachment #8734830 -
Attachment is obsolete: true
Attachment #8734831 -
Attachment is obsolete: true
Attachment #8734886 -
Attachment is obsolete: true
Attachment #8734933 -
Attachment is obsolete: true
Comment 73•8 years ago
|
||
\o/
Comment 74•8 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/06a8c115f8fa for rather clear and honest assertion failures on at least Windows 7 xpcshell, https://treeherder.mozilla.org/logviewer.html#?job_id=24825601&repo=mozilla-inbound, and deeply disingenuous and probably lying crashes that are actually eaten assertions with only the crash vomited up from Mac and Windows debug Marionette, https://treeherder.mozilla.org/logviewer.html#?job_id=24824549&repo=mozilla-inbound and https://treeherder.mozilla.org/logviewer.html#?job_id=24829980&repo=mozilla-inbound
Assignee | ||
Comment 75•8 years ago
|
||
Ok all 3 failures are assertion failures that we don't create any dangling pointers when destroying the SharedImmutableStringsCache (ie, the refcount of each string in the cache is 0). These are all under ~JSRuntime, meaning we have already done a shutdown GC. And there are still `ScriptSource`s hanging around after their JSRuntime has been destroyed. That's pretty surprising and scary in and of itself. All that said, I haven't been able to reproduce locally yet.
Assignee | ||
Comment 76•8 years ago
|
||
Here is a try push without this patch series that puts all ScriptSources into a linked list on the runtime and asserts this list is empty upon destruction. If we are already leaking ScriptSources, it should show up in this push. https://treeherder.mozilla.org/#/jobs?repo=try&revision=41855b016a72
Assignee | ||
Comment 77•8 years ago
|
||
I have a pretty good hunch what may be going on here in bug 1260891.
Assignee | ||
Comment 78•8 years ago
|
||
Try push with the patch from bug 1260891: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26916971a31f
Assignee | ||
Comment 79•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #77) > I have a pretty good hunch what may be going on here in bug 1260891. This was incorrect (although it seems good to take that patch anyways). (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #76) > Here is a try push without this patch series that puts all ScriptSources > into a linked list on the runtime and asserts this list is empty upon > destruction. If we are already leaking ScriptSources, it should show up in > this push. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=41855b016a72 This was right. We are leaking `ScriptSource`s in Marionette tests, somehow: > 18:02:54 ERROR - PROCESS-CRASH | runner.py | application crashed [@ mozilla::LinkedList<js::ScriptSource>::~LinkedList()] > 18:02:54 INFO - Crash dump filename: /var/folders/n5/8vlsdw_12tddqc4b5pnh50t000000w/T/tmp4PXDcq.mozrunner/minidumps/64028645-A8CD-432A-A777-5D8333F3F330.dmp > 18:02:54 INFO - Operating system: Mac OS X > 18:02:54 INFO - 10.10.5 14F27 > 18:02:54 INFO - CPU: amd64 > 18:02:54 INFO - family 6 model 69 stepping 1 > 18:02:54 INFO - 4 CPUs > 18:02:54 INFO - Crash reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS > 18:02:54 INFO - Crash address: 0x0 > 18:02:54 INFO - Process uptime: 8 seconds > 18:02:54 INFO - Thread 0 (crashed) > 18:02:54 INFO - 0 XUL!mozilla::LinkedList<js::ScriptSource>::~LinkedList() [LinkedList.h:41855b016a72 : 329 + 0x0] > 18:02:54 INFO - rax = 0x0000000000000000 rdx = 0x00007fff794851f8 > 18:02:54 INFO - rcx = 0x0000000000000000 rbx = 0x00007fff79485c50 > 18:02:54 INFO - rsi = 0x00004e0000004e00 rdi = 0x00004d0000004e03 > 18:02:54 INFO - rbp = 0x00007fff5fbfee20 rsp = 0x00007fff5fbfee10 > 18:02:54 INFO - r8 = 0x00007fff5fbfedc0 r9 = 0x00007fff78a30300 > 18:02:54 INFO - r10 = 0x0000000000000001 r11 = 0x0000000000000206 > 18:02:54 INFO - r12 = 0x000000010f613000 r13 = 0x000000010f616b78 > 18:02:54 INFO - r14 = 0x000000010f616a48 r15 = 0x000000010f613440 > 18:02:54 INFO - rip = 0x00000001065076f2 > 18:02:54 INFO - Found by: given as instruction pointer in context > 18:02:54 INFO - 1 XUL!JSRuntime::~JSRuntime() [LinkedList.h:41855b016a72 : 328 + 0x5] > 18:02:54 INFO - rbx = 0x000000010f616b18 rbp = 0x00007fff5fbfeea0 > 18:02:54 INFO - rsp = 0x00007fff5fbfee30 r12 = 0x000000010f613000 > 18:02:54 INFO - r13 = 0x000000010f616b78 r14 = 0x000000010f616a48 > 18:02:54 INFO - r15 = 0x000000010f613440 rip = 0x00000001064b59f5 > 18:02:54 INFO - Found by: call frame info Unfortunately, this patch means that these leaks could definitely lead to UAF. Right now, that is just a maybe, depending on if the ScriptSource's data is in the compressed source set or not. :-/
Assignee | ||
Updated•8 years ago
|
Attachment #8736047 -
Attachment is obsolete: true
Assignee | ||
Comment 81•8 years ago
|
||
As per IRL discussion with :jimb, MOZ_ASSERT => MOZ_RELEASE_ASSERT in the case where handles into the cache outlive the cache, since this leads to UAF 100% of the time.
Attachment #8747888 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8747866 -
Attachment is obsolete: true
Assignee | ||
Comment 82•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=239d07fcf4ed
Assignee | ||
Comment 83•8 years ago
|
||
More rebasing to fix bustage.
Attachment #8748262 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8747888 -
Attachment is obsolete: true
Assignee | ||
Comment 84•8 years ago
|
||
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ed125fdef88
Assignee | ||
Comment 85•8 years ago
|
||
Fix bad rebase on top of bug 1219098. Walked through it with jimb in person, so carrying r+.
Attachment #8748419 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8748262 -
Attachment is obsolete: true
Assignee | ||
Comment 86•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68c07764b2c3
Assignee | ||
Comment 87•8 years ago
|
||
As discussed in person today. Will fold into the main patch so as not to break bisection.
Attachment #8749389 -
Flags: review?(jimb)
Assignee | ||
Comment 88•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3edac84751ad
Comment 89•8 years ago
|
||
Comment on attachment 8749389 [details] [diff] [review] Hack around pre-existing leaks in the CC graph and avoid UAF Review of attachment 8749389 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. ::: js/src/vm/Runtime.cpp @@ +490,5 @@ > + > + // TODO bug 1261106: There is a pre-existing leak in Gecko's cycle collected > + // heap graph involving unknown edges holding the C++ half of an XHR object > + // alive (which then holds the JS reflector alive as well) that only happens > + // during shutdown on OSX marionette tests. The introduction of the "during restart", right? @@ +499,5 @@ > + // text past the destruction of the `JSRuntime` and their owning cache, > + // which the cache asserts loudly against. I (fitzgen) spent a month > + // tracking someone else's leak down, and finally couldn't justify any more > + // time on it. Since it only happens during shutdown, and only in a very > + // niche scenario, I'm not *that* worried about it. So, if we detect that You probably want to drop these two sentences; they're not really about the code. People looking at 1261106 will see the story. @@ +503,5 @@ > + // niche scenario, I'm not *that* worried about it. So, if we detect that > + // there are still active borrows, just leak the whole cache, and the OS > + // will clean everything up in a second anyways. I'm not happy with this > + // bandaid, but it is the pragmatic solution and it avoids potential > + // use-after-free. This is probably the most important thing in the comment; consider leading with it. @@ +562,5 @@ > > rtSizes->mathCache += mathCache_ ? mathCache_->sizeOfIncludingThis(mallocSizeOf) : 0; > > rtSizes->sharedImmutableStringsCache += > + sharedImmutableStrings_->sizeOfExcludingThis(mallocSizeOf); Doesn't this want a "sizeOfIncludingThis" now that it's not part of some larger object? ::: js/src/vm/SharedImmutableStringsCache.h @@ +348,5 @@ > + > +#ifdef DEBUG > + // We can only use whether the table is empty or not as a synonym for > + // "are there active borrows?" if table entries are guaranteed never to > + // have a zero refcount. Nice.
Attachment #8749389 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 90•8 years ago
|
||
Addresses review comments and folded back into main patch.
Attachment #8749431 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Attachment #8748419 -
Attachment is obsolete: true
Attachment #8749389 -
Attachment is obsolete: true
Assignee | ||
Comment 91•8 years ago
|
||
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4de5a64b3cbf
Assignee | ||
Comment 92•8 years ago
|
||
Will fold this into the main patch after review to avoid breaking bisection. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22740c5a322b
Attachment #8749802 -
Flags: review?(jimb)
Assignee | ||
Comment 93•8 years ago
|
||
Fix bad header include order that made spidermonkey's check-style complain loudly. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cabc928b1b4b
Attachment #8749803 -
Flags: review?(jimb)
Assignee | ||
Updated•8 years ago
|
Attachment #8749802 -
Attachment is obsolete: true
Attachment #8749802 -
Flags: review?(jimb)
Assignee | ||
Comment 94•8 years ago
|
||
OSX Marionette tests are green, ASAN tests are green. Looks very promising.
Comment 95•8 years ago
|
||
Comment on attachment 8749803 [details] [diff] [review] Hack around shutdown craziness by refcounting the shared immutable strings cache's contents Review of attachment 8749803 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsscript.cpp @@ -46,5 @@ > #include "vm/Debugger.h" > #include "vm/Opcodes.h" > #include "vm/SelfHosting.h" > #include "vm/Shape.h" > -#include "vm/SharedImmutableStringsCache.h" I don't think we're supposed to delete the main #include just because we're #including the inline header. ::: js/src/threading/ExclusiveData.h @@ +104,5 @@ > + */ > + template <typename... Args> > + explicit ExclusiveData(Args&&... args) { > + new (value_.addr()) T(mozilla::Forward<Args>(args)...); > + } Reverence: "WHAT IS THIS BLACK MAGIC" Reverent contempt: "... AND HOW CAN I TREAT ITS EDGE CONDITIONS AS KEY FEATURES" nm ::: js/src/vm/SharedImmutableStringsCache.h @@ +184,5 @@ > + SharedImmutableStringsCache clone() { > + MOZ_ASSERT(inner_); > + auto locked = inner_->lock(); > + locked->refcount++; > + return SharedImmutableStringsCache(inner_); I think it's pretty weird that constructing an instance of the SharedImmutableStringsCache type, which exists to reference-count an ExclusiveData<Inner>, doesn't actually increment the reference count, and it's left to every use of the constructor to take care of that. Granted, that constructor is private.
Attachment #8749803 -
Flags: review?(jimb) → review+
Assignee | ||
Comment 96•8 years ago
|
||
(In reply to Jim Blandy :jimb from comment #95) > I think it's pretty weird that constructing an instance of the > SharedImmutableStringsCache type, which exists to reference-count an > ExclusiveData<Inner>, doesn't actually increment the reference count, and > it's left to every use of the constructor to take care of that. Granted, > that constructor is private. Yes, it is a little strange, but it is required because we sometimes have the lock held when invoking this constructor, and we must avoid reentrant locking attempts. As you point out, it is a private ctor, so not that bad all things considered.
Comment 99•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/98a28a1fce30
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 100•8 years ago
|
||
Am currently having build faliures: c:\t1\hg\objdir-sm\dist\include\js/HashTable.h(1135) : error C2280: 'js::SharedImmutableStringsCache::SharedImmutableStringsCache(const js::SharedImmutableStringsCache &)' : attempting to reference a deleted function c:\t1\hg\comm-central\mozilla\js\src\vm/SharedImmutableStringsCache.h(192) : see declaration of 'js::SharedImmutableStringsCache::SharedImmutableStringsCache' This diagnostic occurred in the compiler generated function 'js::SharedImmutableString::SharedImmutableString(const js::SharedImmutableString &)'
Assignee | ||
Comment 101•8 years ago
|
||
(In reply to Philip Chee from comment #100) > Am currently having build faliures: What compiler and version?
Comment 102•8 years ago
|
||
Visual Studio 2013 Community Edition.
Assignee | ||
Comment 103•8 years ago
|
||
Attachment #8750460 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 104•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #103) > Created attachment 8750460 [details] [diff] [review] > follow up - Explicitly delete implicitly deleted copy constructors for VS2013 Philip: does this patch fix the build on VS2013 for you?
Flags: needinfo?(philip.chee)
Assignee | ||
Comment 105•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #103) > Created attachment 8750460 [details] [diff] [review] > follow up - Explicitly delete implicitly deleted copy constructors for VS2013 Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0088605bc505
Comment 106•8 years ago
|
||
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #104) > (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #103) > > Created attachment 8750460 [details] [diff] [review] > > follow up - Explicitly delete implicitly deleted copy constructors for VS2013 > > Philip: does this patch fix the build on VS2013 for you? No because I still fail at: Bug 1264499
Comment 107•8 years ago
|
||
> // If you want to copy, do it explicitly with `clone`.
> - SharedImmutableStringsCache(const SharedImmutableStringsCache&) = delete;
> + SharedImmutableStringsCache& operator=(const SharedImmutableStringsCache&) = delete;
This one works if I delete the second and third hunks.
Flags: needinfo?(philip.chee)
Comment 108•8 years ago
|
||
> This one works if I delete the second and third hunks.
The patch attached to this comment compiles with VS2013 but of course I don't know if it gives the correct result.
Attachment #8750689 -
Flags: feedback?
Comment 109•8 years ago
|
||
Comment on attachment 8750689 [details] [diff] [review] VS2013 safe patch Oops. https://treeherder.mozilla.org/#/jobs?repo=try&revision=16b1621584a2
Attachment #8750689 -
Flags: feedback? → feedback?(nfitzgerald)
Assignee | ||
Comment 110•8 years ago
|
||
Comment on attachment 8750689 [details] [diff] [review] VS2013 safe patch Review of attachment 8750689 [details] [diff] [review]: ----------------------------------------------------------------- Strange that it doesn't like the deletion of the copy assignment operators... f+ I guess? Waldo?
Attachment #8750689 -
Flags: feedback?(nfitzgerald) → feedback+
Updated•8 years ago
|
Attachment #8750460 -
Flags: review?(jwalden+bmo) → review+
Comment 111•8 years ago
|
||
I assume Waldo meant to r+ attachment 8750689 [details] [diff] [review] ?
Comment 113•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d035a4e34c7b
You need to log in
before you can comment on or make changes to this bug.
Description
•