Bug 172724 - convertEnumerationToJS() should not stash ASCIILiteral strings in NeverDestroyed arrays.
Summary: convertEnumerationToJS() should not stash ASCIILiteral strings in NeverDestro...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-30 10:55 PDT by Mark Lam
Modified: 2017-05-30 13:36 PDT (History)
8 users (show)

See Also:

Attachments
proposed patch. (2.79 KB, patch)
2017-05-30 11:45 PDT, Mark Lam 		mark.lam: review-
Details | Formatted Diff | Diff
proposed patch w/ bindings tests rebaselined. (11.80 KB, patch)
2017-05-30 11:51 PDT, Mark Lam 		cdumez: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (1.58 MB, application/zip)
2017-05-30 13:21 PDT, Build Bot 		no flags Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2017-05-30 10:55:49 PDT 
Use MAKE_STATIC_STRING_IMPL instead.

<rdar://problem/31193201>
Comment 1 Mark Lam 2017-05-30 11:45:00 PDT 
Created attachment 311522 [details]
proposed patch.
Comment 2 Mark Lam 2017-05-30 11:47:56 PDT 
Comment on attachment 311522 [details]
proposed patch.

Need to rebase bindings tests.  Will update patch shortly.
Comment 3 Chris Dumez 2017-05-30 11:48:54 PDT 
Comment on attachment 311522 [details]
proposed patch.

You will likely need to rebaseline the bindings tests.
Comment 4 Mark Lam 2017-05-30 11:51:40 PDT 
Created attachment 311525 [details]
proposed patch w/ bindings tests rebaselined.
Comment 5 Chris Dumez 2017-05-30 11:53:06 PDT 
Comment on attachment 311525 [details]
proposed patch w/ bindings tests rebaselined.

r=me
Comment 6 Joseph Pecoraro 2017-05-30 11:54:20 PDT 
View in context: https://bugs.webkit.org/attachment.cgi?id=311525&action=review

r=me as well

> Source/WebCore/ChangeLog:17
> +        whose root cause is not known yet.  This patch also adds a release assert to

Nit: One space after a period!
Comment 7 Mark Lam 2017-05-30 11:55:47 PDT 
(In reply to Joseph Pecoraro from comment #6)
> > Source/WebCore/ChangeLog:17
> > +        whose root cause is not known yet.  This patch also adds a release assert to
> 
> Nit: One space after a period!

Not in a ChangeLog.  In a ChangeLog, I'm permitted to use the more readable style of 2 spaces between sentences. =)
Comment 8 Build Bot 2017-05-30 13:21:24 PDT 
Comment on attachment 311525 [details]
proposed patch w/ bindings tests rebaselined.

Attachment 311525 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3843663

New failing tests:
imported/w3c/web-platform-tests/innerText/getter.html
Comment 9 Build Bot 2017-05-30 13:21:26 PDT 
Created attachment 311535 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 10 Mark Lam 2017-05-30 13:29:20 PDT 
(In reply to Build Bot from comment #8)
> Comment on attachment 311525 [details]
> proposed patch w/ bindings tests rebaselined.
> 
> Attachment 311525 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3843663
> 
> New failing tests:
> imported/w3c/web-platform-tests/innerText/getter.html

This is not due to my patch.  See https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r217560%20(1262)/results.html.  This issue is already failing in trunk without my patch.
Comment 11 Mark Lam 2017-05-30 13:36:01 PDT 
Thanks for the review.  Landed in r217568: <http://trac.webkit.org/r217568>.