Use SecurityOriginData as keys in StorageManager
Created attachment 283792 [details] Patch
Attachment 283792 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 283794 [details] Patch
Attachment 283794 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 283796 [details] Patch
Attachment 283796 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 283803 [details] Patch
Attachment 283803 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 283813 [details] Patch
Attachment 283813 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 283816 [details] Patch
Attachment 283816 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 283816 [details] Patch Attachment 283816 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1688364 Number of test failures exceeded the failure limit.
Created attachment 283828 [details] Archive of layout-test-results from ews115 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 283829 [details] Patch
Attachment 283829 [details] did not pass style-queue: ERROR: Source/WebKit2/UIProcess/Storage/StorageManager.cpp:435: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 283829 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=283829&action=review > Source/WebCore/page/SecurityOrigin.cpp:167 > + // FIXME: We should only use this on the main thread. I don’t understand this comment. What is "this" referring to? The SecurityOrigin::create function? The shouldTreatAsUniqueOrigin function? What is the fix for the problem the comment is describing? Why are we not making the fix at this time? > Source/WebCore/page/SecurityOrigin.cpp:169 > Ref<SecurityOrigin> origin(adoptRef(*new SecurityOrigin)); We should change this line of code to: auto origin = adoptRef(*new SecurityOrigin); > Source/WebCore/page/SecurityOriginData.cpp:110 > +bool operator==(const SecurityOrigin& a, const SecurityOriginData& b) > +{ > + return a.protocol() == b.protocol > + && a.host() == b.host > + && a.port() == b.port; > +} > + > +bool operator==(const SecurityOriginData& a, const SecurityOrigin& b) > +{ > + return a.protocol == b.protocol() > + && a.host == b.host() > + && a.port == b.port(); > +} It doesn’t seem right to use the == operation for this. There are so many other fields in a SecurityOrigin that this is ignoring. So this is some kind of matching operation, not an == operation. > Source/WebCore/page/SecurityOriginData.h:59 > +WEBCORE_EXPORT bool operator==(const SecurityOrigin&, const SecurityOriginData&); > +WEBCORE_EXPORT bool operator==(const SecurityOriginData&, const SecurityOrigin&); We should find another solution to whatever problem we are solving by adding these == operators. There are other techniques for using keys of different types in collections.
(In reply to comment #17) > Comment on attachment 283829 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=283829&action=review > > > Source/WebCore/page/SecurityOrigin.cpp:167 > > + // FIXME: We should only use this on the main thread. > > I don’t understand this comment. What is "this" referring to? The > SecurityOrigin::create function? The shouldTreatAsUniqueOrigin function? shouldTreatAsUniqueOrigin > > What is the fix for the problem the comment is describing? Why are we not > making the fix at this time? There are still uses of shouldTreatAsUniqueOrigin on non-main threads that are outside the scope of this patch to fix.
Comment on attachment 283829 [details] Patch I think you need to address Darin's comments before this is ready for review again. The operator== use is pretty concerning (marking objects are equal when they have different data fields, or when they're not even the same object)!
Created attachment 294430 [details] Patch
Created attachment 294440 [details] Patch
Created attachment 294442 [details] Patch
Comment on attachment 294442 [details] Patch Attachment 294442 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2494160 Number of test failures exceeded the failure limit.
Created attachment 294450 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 294442 [details] Patch Attachment 294442 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2494167 Number of test failures exceeded the failure limit.
Created attachment 294451 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 294442 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=294442&action=review > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:534 > - origins.add(WTFMove(origin)); > + origins.add(origin); Is this intentional? > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:577 > - origins.add(WTFMove(origin)); > + origins.add(origin); Ditto. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:581 > - origins.add(WTFMove(origin)); > + origins.add(origin); Ditto. > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:691 > - if (!origin->isSameSchemeHostPort(&area->securityOrigin())) > + if (!(origin == area->securityOrigin())) This is a change in behavior. Is this intentional? I mean, SecurityOrigin::isSameSchemeHostPort() has special logic for file-based origins. > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:414 > + websiteData.entries.append(WebsiteData::Entry { WTFMove(origin).securityOrigin().ptr(), WebsiteDataType::MediaKeys, 0 }); Is the WTFMove() needed? > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:790 > + for (auto origin : origins) Did you mean to use "auto" to copy each SecurityOriginData object instead of "auto&"?
Created attachment 294458 [details] Patch
Comment on attachment 294458 [details] Patch Attachment 294458 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2494697 Number of test failures exceeded the failure limit.
Created attachment 294460 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 294458 [details] Patch Attachment 294458 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2494704 Number of test failures exceeded the failure limit.
Created attachment 294461 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
(In reply to comment #27) > Comment on attachment 294442 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=294442&action=review > > > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:534 > > - origins.add(WTFMove(origin)); > > + origins.add(origin); > > Is this intentional? Yes, the type is now const SecurityOriginData& instead of RefPtr<SecurityOrigin>. > > Source/WebKit2/UIProcess/Storage/StorageManager.cpp:691 > > - if (!origin->isSameSchemeHostPort(&area->securityOrigin())) > > + if (!(origin == area->securityOrigin())) > > This is a change in behavior. Is this intentional? I mean, > SecurityOrigin::isSameSchemeHostPort() has special logic for file-based > origins. I won't change this behavior. Thanks. > > > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:414 > > + websiteData.entries.append(WebsiteData::Entry { WTFMove(origin).securityOrigin().ptr(), WebsiteDataType::MediaKeys, 0 }); > > Is the WTFMove() needed? No > > > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:790 > > + for (auto origin : origins) > > Did you mean to use "auto" to copy each SecurityOriginData object instead of > "auto&"? No, thanks. There's also some other issue causing a lot of crashing. Luckily it's easy to reproduce.
Created attachment 294557 [details] Patch
Comment on attachment 294557 [details] Patch After EWS, sure.
Comment on attachment 294557 [details] Patch Clearing flags on attachment: 294557 Committed r208633: <http://trac.webkit.org/changeset/208633>
All reviewed patches have been landed. Closing bug.