ObjectPropertyCondition and Graph::tryGetConstantProperty should lock the structure before calling getDirect
Created attachment 343405 [details] Patch
Comment on attachment 343405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343405&action=review > Source/JavaScriptCore/bytecode/PropertyCondition.cpp:400 > + // If we are flattening the structure at this time we might shrink the butterfly and read garbage. > + ConcurrentJSLocker locker(structure->lock()); > + if (!structure->isValidOffset(offset())) > + return PropertyCondition(); > + value = base->getDirect(offset()); Don't we know if we're doing this concurrently or not? Maybe just lock in that scenario?
Comment on attachment 343405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343405&action=review >> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:400 >> + value = base->getDirect(offset()); > > Don't we know if we're doing this concurrently or not? Maybe just lock in that scenario? Fair point, I'll make that change.
Comment on attachment 343405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343405&action=review >>> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:400 >>> + value = base->getDirect(offset()); >> >> Don't we know if we're doing this concurrently or not? Maybe just lock in that scenario? > > Fair point, I'll make that change. Actually, we do a lot of things using the concurrent versions of methods inside PropertyCondition when we are on the mutator. We should fix all of them in a more consistent way. I'm just gonna file a bug.
Created attachment 343410 [details] Patch
Created attachment 343411 [details] Patch
rdar://problem/41385437
Comment on attachment 343411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343411&action=review r=me. If you haven't already done so, I think you should also remove the "if (!value)" check I added in PropertyCondition::isValidValueForAttributes() due to https://bugs.webkit.org/show_bug.cgi?id=186943. Your changes in attemptToMakeEquivalenceWithoutBarrier() makes that unnecessary. > Source/JavaScriptCore/dfg/DFGGraph.cpp:1268 > + // object pointer such that the inlinecaches told us that the object had a structure that it missing space between "inline" and "caches". > Source/JavaScriptCore/dfg/DFGGraph.cpp:1270 > + // caches had alraedy seen. And then the processor reordered the stores. Seems unlikely and /alraedy/already/.
Comment on attachment 343411 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=343411&action=review >> Source/JavaScriptCore/dfg/DFGGraph.cpp:1268 >> + // object pointer such that the inlinecaches told us that the object had a structure that it > > missing space between "inline" and "caches". Fixed >> Source/JavaScriptCore/dfg/DFGGraph.cpp:1270 >> + // caches had alraedy seen. And then the processor reordered the stores. Seems unlikely and > > /alraedy/already/. Fixed
(In reply to Mark Lam from comment #8) > r=me. If you haven't already done so, I think you should also remove the > "if (!value)" check I added in > PropertyCondition::isValidValueForAttributes() due to > https://bugs.webkit.org/show_bug.cgi?id=186943. Your changes in > attemptToMakeEquivalenceWithoutBarrier() makes that unnecessary. I think I'm just going to undo my change to attemptToMakeEquivalenceWithoutBarrier() instead. That check makes more sense in isValidValueForAttributes().
Committed r233124: <https://trac.webkit.org/changeset/233124>