Fix bugs from r228411

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Saam Barati]

...

[Saam Barati]

<rdar://problem/37577732>

[Saam Barati]

Created attachment 333974 [details]
patch

[EWS Watchlist]

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.

[Saam Barati]

(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.

[JF Bastien]

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?

[Saam Barati]

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.

[Saam Barati]

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

[Saam Barati]

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.

[Saam Barati]

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.

[Saam Barati]

Created attachment 333983 [details]
patch for landing

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Filip Pizlo]

Looks like the failure probably has nothing to do with this patch.

[Saam Barati]

(In reply to Filip Pizlo from comment #14)
> Looks like the failure probably has nothing to do with this patch.

Agreed. Ima cq+

[WebKit Commit Bot]

Comment on attachment 333983 [details]
patch for landing

Clearing flags on attachment: 333983

Committed r228565: <https://trac.webkit.org/changeset/228565>