Bug 195435 - Make sure an empty host matches the internal representation "nullOrigin" in WebCore::RegistrableDomain::matches()
Summary: Make sure an empty host matches the internal representation "nullOrigin" in W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-07 14:28 PST by John Wilander
Modified: 2019-03-11 11:23 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2019-03-07 14:31 PST, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-03-07 14:28:09 PST
WebCore::RegistrableDomain::matches() is a quick way to compare a RegistrableDomain with a URL. Since RegistrableDomain represents the empty host as "nullOrigin," a URL with an empty host needs to match that representation in WebCore::RegistrableDomain::matches(). This omission caused debug assertions in fast/ layout tests after https://trac.webkit.org/changeset/242603/webkit.
Comment 1 John Wilander 2019-03-07 14:31:57 PST
Created attachment 363929 [details]
Patch
Comment 2 Brent Fulgham 2019-03-07 15:56:53 PST
Comment on attachment 363929 [details]
Patch

r=me
Comment 3 WebKit Commit Bot 2019-03-07 16:23:57 PST
Comment on attachment 363929 [details]
Patch

Clearing flags on attachment: 363929

Committed r242620: <https://trac.webkit.org/changeset/242620>
Comment 4 WebKit Commit Bot 2019-03-07 16:23:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2019-03-07 16:25:15 PST
<rdar://problem/48695077>
Comment 6 Darin Adler 2019-03-08 12:54:54 PST
Comment on attachment 363929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363929&action=review

> Source/WebCore/platform/RegistrableDomain.h:62
> +        if (host.isEmpty() && m_registrableDomain == "nullOrigin"_s)

Using _s with == doesn’t optimize things; I think in fact it just makes them slower. This _s should be omitted.
Comment 7 Darin Adler 2019-03-08 12:55:29 PST
Comment on attachment 363929 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363929&action=review

>> Source/WebCore/platform/RegistrableDomain.h:62
>> +        if (host.isEmpty() && m_registrableDomain == "nullOrigin"_s)
> 
> Using _s with == doesn’t optimize things; I think in fact it just makes them slower. This _s should be omitted.

Or we could fix WTF::String so it at least doesn’t make things slower and then leave this alone.
Comment 8 John Wilander 2019-03-08 13:02:59 PST
(In reply to Darin Adler from comment #6)
> Comment on attachment 363929 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363929&action=review
> 
> > Source/WebCore/platform/RegistrableDomain.h:62
> > +        if (host.isEmpty() && m_registrableDomain == "nullOrigin"_s)
> 
> Using _s with == doesn’t optimize things; I think in fact it just makes them
> slower. This _s should be omitted.

Thanks! Did not know that.
Comment 9 Simon Fraser (smfr) 2019-03-10 09:50:35 PDT
Still lots of asserts in iOS sim:
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29?numbuilds=100
Comment 10 John Wilander 2019-03-10 12:14:57 PDT
(In reply to Simon Fraser (smfr) from comment #9)
> Still lots of asserts in iOS sim:
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Debug%20WK2%20%28Tests%29?numbuilds=100

Sorry about that. I will have a look tomorrow morning. It’s probably best to restructure the check before the IPC call. The reason for all of this is that I don’t want to do IPC for useless cases, only to drop them on the other side.
Comment 11 John Wilander 2019-03-11 10:22:17 PDT
Looking at this now.
Comment 12 John Wilander 2019-03-11 11:19:43 PDT
These assertion failures seem unrelated. The problem seems to be in WebKit::RemoteLayerBackingStoreCollection::backingStoreWillBeDisplayed(). Or did you see my code causing this, Simon?

ASSERTION FAILED: m_inLayerFlush
/Volumes/Data/slave/ios-simulator-12-debug/build/Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStoreCollection.mm(92) : bool WebKit::RemoteLayerBackingStoreCollection::backingStoreWillBeDisplayed(WebKit::RemoteLayerBackingStore &)
Comment 13 John Wilander 2019-03-11 11:23:06 PDT
I was not able to repro with ToT on my machine, executing:
run-webkit-tests --ios-simulator animations/animation-controller-drt-api.html --no-retry-failures