Bug 28262 – SecurityOrigin::createFromDatabaseIdentifier contains bugs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

SecurityOrigin::createFromDatabaseIdentifier contains the following two bugs.

Line 286
When testing that the identifier contains at least two separators, the test is reversed. As a result, identifiers with at least two separators are erroneously rejected.
This was introduced in http://trac.webkit.org/changeset/44423.

Test case ...
INPUT:    "http_webkit.org_80"
ACTUAL:   SecurityOrigin(KURL()) (input rejected)
EXPECTED: SecurityOrigin(KURL("http://webkit.org:80"))

Line 292
When testing that the port is either a valid integer or absent, the logic is incorrect. Instead, the logic tests that the port is a valid integer or present. Hence identifiers where the port is absent are erroneously rejected and those where the port is present but invalid are erroneously accepted.
This was introduced in http://trac.webkit.org/changeset/29386.

Test cases ...
INPUT:    "http_webkit.org_"
ACTUAL:   SecurityOrigin(KURL()) (input rejected)
EXPECTED: SecurityOrigin(KURL("http://webkit.org:0"))

INPUT:    "http_webkit.org_string"
ACTUAL:   SecurityOrigin(KURL("http://webkit.org:0"))
EXPECTED: SecurityOrigin(KURL()) (input rejected)

[Jeremy Orlow]

I'll take a look.

[Steve Block]

Created attachment 34753 [details]
Patch 1 for bug 28262

Is there a way to add low-level tests for this kind of functionality?

[Jeremy Orlow]

(In reply to comment #2)
> Created an attachment (id=34753) [details]
> Patch 1 for bug 28262
> 
> Is there a way to add low-level tests for this kind of functionality?

Not really.  And, in fact, I created a bug suggesting that we do find a way.  This class seems trivial to unittests, but ther'es no framework at the moment.  (Only LayoutTests.)

[Darin Adler]

Comment on attachment 34753 [details]
Patch 1 for bug 28262

The best way to make tests for this is to figure out what actual effect the bugs would have. It's OK to have them not tested if there's no obvious way to do so. Building a lower level unit test machinery would allow testing things like this, but would have other costs that may not be acceptable.

Find to just land this as-is. r=me

[Steve Block]

Assigning to benm for landing.

[Eric Seidel (no email)]

Comment on attachment 34753 [details]
Patch 1 for bug 28262

Clearing flags on attachment: 34753

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/page/SecurityOrigin.cpp
Committed r47221
	M	WebCore/ChangeLog
	M	WebCore/page/SecurityOrigin.cpp
r47221 = ce243898041701dab28e73a78b800546022b62dc (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/47221

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.