Bug 159834 – Use SecurityOriginData as keys in StorageManager

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use SecurityOriginData as keys in StorageManager

[Alex Christensen]

Created attachment 283792 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

Created attachment 283794 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

Created attachment 283796 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

Created attachment 283803 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

Created attachment 283813 [details]
Patch

[WebKit Commit Bot]

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.

[Alex Christensen]

Created attachment 283816 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Alex Christensen]

Created attachment 283829 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Alex Christensen]

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

[Michael Catanzaro]

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)!

[Alex Christensen]

Created attachment 294430 [details]
Patch

[Alex Christensen]

Created attachment 294440 [details]
Patch

[Alex Christensen]

Created attachment 294442 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Daniel Bates]

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&"?

[Alex Christensen]

Created attachment 294458 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Alex Christensen]

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

[Alex Christensen]

Created attachment 294557 [details]
Patch

[Brady Eidson]

Comment on attachment 294557 [details]
Patch

After EWS, sure.

[WebKit Commit Bot]

Comment on attachment 294557 [details]
Patch

Clearing flags on attachment: 294557

Committed r208633: <http://trac.webkit.org/changeset/208633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.