...
<rdar://problem/37577732>
Created attachment 333974 [details] patch
Attachment 333974 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:530: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #3) > Attachment 333974 [details] did not pass style-queue: > > > ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use > of security-sensitive phrasing could help someone exploit WebKit: security > bug [changelog/unwantedsecurityterms] [3] This fixes bug from a patch that landed earlier this week. I'm going to keep the word "security" in the changelog since it's descriptive. > ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:530: This { should be at the > end of the previous line [whitespace/braces] [4] > Total errors found: 2 in 11 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style.
Comment on attachment 333974 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333974&action=review r=me > JSTests/stress/constant-folding-phase-insert-check-handle-varargs.js:7 > + return properties[seed % properties.length]; I usually inline the object / properties because then the repro case isn't random anymore. > Source/JavaScriptCore/dfg/DFGGraph.h:529 > + AdjacencyList copyVarargChildren(Node* node, WTF::Function<bool(Edge)> filter = [] (Edge) { return true; }) I think it's better to pass the function as a template param here, so you never have a WTF::Function ever. > Source/JavaScriptCore/dfg/DFGPureValue.h:107 > + hash ^= m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized().hash(); Sanitized hash is important here? Why?
Comment on attachment 333974 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333974&action=review >> JSTests/stress/constant-folding-phase-insert-check-handle-varargs.js:7 >> + return properties[seed % properties.length]; > > I usually inline the object / properties because then the repro case isn't random anymore. The repro case here isn't random. It doesn't depend on the data. I believe it just depends on having the entire program tier up. It crashes 100% of the time with CJIT off. >> Source/JavaScriptCore/dfg/DFGGraph.h:529 >> + AdjacencyList copyVarargChildren(Node* node, WTF::Function<bool(Edge)> filter = [] (Edge) { return true; }) > > I think it's better to pass the function as a template param here, so you never have a WTF::Function ever. Sounds good. >> Source/JavaScriptCore/dfg/DFGPureValue.h:107 >> + hash ^= m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized().hash(); > > Sanitized hash is important here? Why? Edge is basically a tuple of some metadata bits and Node*. But, we encode those bits in the upper bits of the Node* pointer. sanitized() just kills various, but not all, of those metadata bits. That makes sense for CSE, since for example, some edges may have different proof status bits. But that wouldn't imply that we couldn't CSE the owner of that edge together.
Comment on attachment 333974 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333974&action=review >>> Source/JavaScriptCore/dfg/DFGGraph.h:529 >>> + AdjacencyList copyVarargChildren(Node* node, WTF::Function<bool(Edge)> filter = [] (Edge) { return true; }) >> >> I think it's better to pass the function as a template param here, so you never have a WTF::Function ever. > > Sounds good. I remember why I did this: the compiler can't infer the type of the template parameter when not called with a filter function. Not sure why? I guess I can specify a default type for the template parameter too, but I guess that leaves open the opportunity we actually end up with a WTF::Function
Comment on attachment 333974 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333974&action=review >>>> Source/JavaScriptCore/dfg/DFGGraph.h:529 >>>> + AdjacencyList copyVarargChildren(Node* node, WTF::Function<bool(Edge)> filter = [] (Edge) { return true; }) >>> >>> I think it's better to pass the function as a template param here, so you never have a WTF::Function ever. >> >> Sounds good. > > I remember why I did this: the compiler can't infer the type of the template parameter when not called with a filter function. Not sure why? I guess I can specify a default type for the template parameter too, but I guess that leaves open the opportunity we actually end up with a WTF::Function actually, I can probably make the default type a C function pointer.
Comment on attachment 333974 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333974&action=review >>> Source/JavaScriptCore/dfg/DFGPureValue.h:107 >>> + hash ^= m_graph->m_varArgChildren[m_children.firstChild() + i].sanitized().hash(); >> >> Sanitized hash is important here? Why? > > Edge is basically a tuple of some metadata bits and Node*. But, we encode those bits in the upper bits of the Node* pointer. sanitized() just kills various, but not all, of those metadata bits. That makes sense for CSE, since for example, some edges may have different proof status bits. But that wouldn't imply that we couldn't CSE the owner of that edge together. To clarify, when we're not in varargs mode, we sanitize m_children on construction of the PureValue. In this mode, we do it on demand, since there is no way to do it on construction without allocating extra memory.
Created attachment 333983 [details] patch for landing
Attachment 333983 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/ChangeLog:12: Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: security bug [changelog/unwantedsecurityterms] [3] ERROR: Source/JavaScriptCore/dfg/DFGGraph.h:531: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 333983 [details] patch for landing Attachment 333983 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6528600 New failing tests: http/tests/preload/onerror_event.html
Created attachment 333994 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Looks like the failure probably has nothing to do with this patch.
(In reply to Filip Pizlo from comment #14) > Looks like the failure probably has nothing to do with this patch. Agreed. Ima cq+
Comment on attachment 333983 [details] patch for landing Clearing flags on attachment: 333983 Committed r228565: <https://trac.webkit.org/changeset/228565>