...
Created attachment 334848 [details] patch
<rdar://problem/36245022>
Comment on attachment 334848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=334848&action=review r=me > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1200 > + auto isInEffectedGlobalObject = [&] (JSObject* object) { > + for (JSObject* current = object; ;) { > + if (current->globalObject() == m_globalObject) > + return true; > + > + JSValue prototypeValue = current->getPrototypeDirect(vm); > + if (prototypeValue.isNull()) > + return false; > + current = asObject(prototypeValue); > + } > + }; I think you need a "return false" after the for loop in order to be equivalent to the original code. But in this case, I think we should never get there. So, let's have a RELEASE_ASSERT_NOT_REACHED() there. > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211 > + bool isRelevantGlobalObject = structure->globalObject() == m_globalObject I suggest putting parens around the == expr.
Comment on attachment 334848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=334848&action=review >> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211 >> + bool isRelevantGlobalObject = structure->globalObject() == m_globalObject > > I suggest putting parens around the == expr. For stylistic reasons?
Comment on attachment 334848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=334848&action=review >>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211 >>> + bool isRelevantGlobalObject = structure->globalObject() == m_globalObject >> >> I suggest putting parens around the == expr. > > For stylistic reasons? For clarity, and for not relying on operator precedence to ensure correctness.
(In reply to Mark Lam from comment #5) > Comment on attachment 334848 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=334848&action=review > > >>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1211 > >>> + bool isRelevantGlobalObject = structure->globalObject() == m_globalObject > >> > >> I suggest putting parens around the == expr. > > > > For stylistic reasons? > > For clarity, and for not relying on operator precedence to ensure > correctness. I’m fine with this change. However, I don’t think the latter is something we normally do. See: math in most places in WK
Comment on attachment 334848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=334848&action=review > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1227 > + if (isInEffectedGlobalObject(object)) This is broken here. Should be inverted.
Comment on attachment 334848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=334848&action=review >> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1227 >> + if (isInEffectedGlobalObject(object)) > > This is broken here. Should be inverted. Sorry I missed that.
Comment on attachment 334848 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=334848&action=review >>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:1227 >>> + if (isInEffectedGlobalObject(object)) >> >> This is broken here. Should be inverted. > > Sorry I missed that. No worries. I meant to move the m_foundObjects.append to here, but forgot to. Good thing jsc tests found it.
Created attachment 334856 [details] patch for landing
Created attachment 334857 [details] patch for landing
Comment on attachment 334857 [details] patch for landing Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6726252 New failing tests: webrtc/video-replace-muted-track.html
Created attachment 334864 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
(In reply to Build Bot from comment #12) > Comment on attachment 334857 [details] > patch for landing > > Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.webkit.org/results/6726252 > > New failing tests: > webrtc/video-replace-muted-track.html Seems unrelated, but it happened twice. Ima build and try to reproduce.
(In reply to Saam Barati from comment #14) > (In reply to Build Bot from comment #12) > > Comment on attachment 334857 [details] > > patch for landing > > > > Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2): > > Output: http://webkit-queues.webkit.org/results/6726252 > > > > New failing tests: > > webrtc/video-replace-muted-track.html > > Seems unrelated, but it happened twice. Ima build and try to reproduce. Reading the test, this seems super unlikely. However, I'll wait until I have a full build to verify.
(In reply to Saam Barati from comment #15) > (In reply to Saam Barati from comment #14) > > (In reply to Build Bot from comment #12) > > > Comment on attachment 334857 [details] > > > patch for landing > > > > > > Attachment 334857 [details] did not pass mac-wk2-ews (mac-wk2): > > > Output: http://webkit-queues.webkit.org/results/6726252 > > > > > > New failing tests: > > > webrtc/video-replace-muted-track.html > > > > Seems unrelated, but it happened twice. Ima build and try to reproduce. > > Reading the test, this seems super unlikely. However, I'll wait until I have > a full build to verify. This test doesn't "have a bad time" so I have no idea how my change could effect it. I'm going to CQ+.
Comment on attachment 334857 [details] patch for landing Clearing flags on attachment: 334857 Committed r229161: <https://trac.webkit.org/changeset/229161>
All reviewed patches have been landed. Closing bug.