| Summary: | CSS Selector an-plus-b serialization is incorrect | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Keith Miller <keith_miller> | ||||||||||||||
| Component: | New Bugs | Assignee: | Keith Miller <keith_miller> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | darin, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, keith_miller, macpherson, menard, sam, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Keith Miller
2020-09-29 15:56:50 PDT
Created attachment 410063 [details]
Patch
Comment on attachment 410063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410063&action=review > Source/WebCore/css/parser/CSSSelectorParser.cpp:763 > + if (!ab.first) > + return commonVM().numericStrings.add(ab.second); Why is the JS VM involved here? That seems almost certainly wrong. Created attachment 410064 [details]
Patch
Comment on attachment 410063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410063&action=review >> Source/WebCore/css/parser/CSSSelectorParser.cpp:763 >> + return commonVM().numericStrings.add(ab.second); > > Why is the JS VM involved here? That seems almost certainly wrong. Would recommend reading the ChangeLog :P Created attachment 410077 [details]
Patch
Created attachment 410134 [details]
Patch
(In reply to Keith Miller from comment #4) > Comment on attachment 410063 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410063&action=review > > >> Source/WebCore/css/parser/CSSSelectorParser.cpp:763 > >> + return commonVM().numericStrings.add(ab.second); > > > > Why is the JS VM involved here? That seems almost certainly wrong. > > Would recommend reading the ChangeLog :P This doesn't seem like a good idea to me. In general, I don't think utilizing internal data structures of JavaScriptCore outside of the bindings (and the purpose driven JITs) is a good idea in WebCore code. If this optimization is really needed, we should probably find a way to make it work without involving JSC. Comment on attachment 410063 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410063&action=review >>>> Source/WebCore/css/parser/CSSSelectorParser.cpp:763 >>>> + return commonVM().numericStrings.add(ab.second); >>> >>> Why is the JS VM involved here? That seems almost certainly wrong. >> >> Would recommend reading the ChangeLog :P > > This doesn't seem like a good idea to me. In general, I don't think utilizing internal data structures of JavaScriptCore outside of the bindings (and the purpose driven JITs) is a good idea in WebCore code. If this optimization is really needed, we should probably find a way to make it work without involving JSC. I don't know if I would call this an internal data structure of JSC anymore than the AtomicString table is an internal JSC data structure. That said, I did it mostly because I figured it would be simple enough, so I'm happy to remove it. Created attachment 410158 [details]
Patch
Created attachment 410159 [details]
Patch
Committed r267812: <https://trac.webkit.org/changeset/267812> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410159 [details]. |