Fix GetOriginURLFromIdentifier

Fix ObfuscatedOriginEnumerator::Next not to try to call
GetOriginURLFromIdentifier if it doesn't have an identifier.


BUG=133402
TEST=ObfuscatedFileUtilTest.TestCreateAndDeleteFile on Linux debug


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145766

[tzik, 2012-06-19 02:20:16]

On 2012/06/18 23:26:48, ericu wrote:

I couldn't reproduce the crash in my machine and couldn't understand why
previous code causes crash.
Could you point where the test crash at for me?

[ericu, 2012-06-19 16:47:36]

On 2012/06/19 02:20:16, tzik wrote:
> On 2012/06/18 23:26:48, ericu wrote:
> 
> I couldn't reproduce the crash in my machine and couldn't understand why
> previous code causes crash.
> Could you point where the test crash at for me?

During test teardown, we enumerate origins.  When we hit the end of the actual
origins, we end up trying to create a GURL from a string that's not a valid URL.
 That's fine, but then we have a hack that checks gurl.spec().length() to see if
it's empty.  It's not legal to take the spec() of an invalid GURL, so we assert.
 The code does the right thing in release mode, so nothing fails in release.

Now that I think of it, we really just shouldn't check spec().length() there
either.  I'm uploading an additional change to fix that as well.

MichaelN suspects this new failure has something to do with the test moving to
content_unittests, which has a message loop, whereas test_shell_tests apparently
doesn't.  Maybe it just wasn't doing the cleanup at all before because of that? 
A number of the tests [including at least one in Quota] will now crash here for
me, on a clean checkout.

[6713:6713:0619/093459:1105790947825:FATAL:gurl.cc(200)] Check failed: false.
Trying to get the spec of an invalid URL!
Backtrace:
	base::debug::StackTrace::StackTrace() [0xe67842]
	logging::LogMessage::~LogMessage() [0xde620f]
	GURL::spec() [0x1a41e0a]
	fileapi::GetOriginURLFromIdentifier() [0x1875406]
	fileapi::ObfuscatedOriginEnumerator::Next() [0x18890fe]
	fileapi::(anonymous namespace)::ObfuscatedOriginEnumerator::Next() [0x188fbe1]
	fileapi::SandboxMountPointProvider::GetOriginsForHostOnFileThread() [0x189259e]
	fileapi::FileSystemQuotaClient::GetOriginsForHostTask::RunOnTargetThread()
[0x1867a0e]
	quota::QuotaThreadTask::RunOnTargetThreadAsync() [0x24e7ab7]
	quota::QuotaThreadTask::CallRunOnTargetThread() [0x24e79ef]
	base::internal::RunnableAdapter<>::Run() [0x24e96f1]
	base::internal::InvokeHelper<>::MakeItSo() [0x24e92dc]
	base::internal::Invoker<>::Run() [0x24e8b63]
	base::Callback<>::Run() [0x46ebe3]
	MessageLoop::RunTask() [0xdeb64c]
	MessageLoop::DeferOrRunPendingTask() [0xdeb763]
	MessageLoop::DoWork() [0xdebf57]
	base::MessagePumpDefault::Run() [0xdf3dc0]
	MessageLoop::RunInternal() [0xdeb313]
	MessageLoop::RunHandler() [0xdeb1ca]
	MessageLoop::RunAllPending() [0xdeab48]
	fileapi::FileSystemTestOriginHelper::TearDown() [0x87bcda]
	ObfuscatedFileUtilTest::TearDown() [0x8cbbed]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1d40986]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1d3d9f8]
	testing::Test::Run() [0x1d325c9]
	testing::TestInfo::Run() [0x1d32da4]
	testing::TestCase::Run() [0x1d3349a]
	testing::internal::UnitTestImpl::RunAllTests() [0x1d38425]
	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1d417d8]
	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1d3e227]
	testing::UnitTest::Run() [0x1d36f0e]
	base::TestSuite::Run() [0xd45081]
	content::UnitTestTestSuite::Run() [0xb176cc]
	main [0x6e7e96]
	0x7fedd37dfc4d
	0x4293f9

[ericu, 2012-06-19 17:33:20]

On 2012/06/19 16:47:36, ericu wrote:
> On 2012/06/19 02:20:16, tzik wrote:
> > On 2012/06/18 23:26:48, ericu wrote:
> > 
> > I couldn't reproduce the crash in my machine and couldn't understand why
> > previous code causes crash.
> > Could you point where the test crash at for me?
> 
> During test teardown, we enumerate origins.  When we hit the end of the actual
> origins, we end up trying to create a GURL from a string that's not a valid
URL.
>  That's fine, but then we have a hack that checks gurl.spec().length() to see
if
> it's empty.  It's not legal to take the spec() of an invalid GURL, so we
assert.
>  The code does the right thing in release mode, so nothing fails in release.
> 
> Now that I think of it, we really just shouldn't check spec().length() there
> either.  I'm uploading an additional change to fix that as well.
> 
> MichaelN suspects this new failure has something to do with the test moving to
> content_unittests, which has a message loop, whereas test_shell_tests
apparently
> doesn't.  Maybe it just wasn't doing the cleanup at all before because of
that? 
> A number of the tests [including at least one in Quota] will now crash here
for
> me, on a clean checkout.
> 
> [6713:6713:0619/093459:1105790947825:FATAL:gurl.cc(200)] Check failed: false.
> Trying to get the spec of an invalid URL!
> Backtrace:
> 	base::debug::StackTrace::StackTrace() [0xe67842]
> 	logging::LogMessage::~LogMessage() [0xde620f]
> 	GURL::spec() [0x1a41e0a]
> 	fileapi::GetOriginURLFromIdentifier() [0x1875406]
> 	fileapi::ObfuscatedOriginEnumerator::Next() [0x18890fe]
> 	fileapi::(anonymous namespace)::ObfuscatedOriginEnumerator::Next()
[0x188fbe1]
> 	fileapi::SandboxMountPointProvider::GetOriginsForHostOnFileThread()
[0x189259e]
> 	fileapi::FileSystemQuotaClient::GetOriginsForHostTask::RunOnTargetThread()
> [0x1867a0e]
> 	quota::QuotaThreadTask::RunOnTargetThreadAsync() [0x24e7ab7]
> 	quota::QuotaThreadTask::CallRunOnTargetThread() [0x24e79ef]
> 	base::internal::RunnableAdapter<>::Run() [0x24e96f1]
> 	base::internal::InvokeHelper<>::MakeItSo() [0x24e92dc]
> 	base::internal::Invoker<>::Run() [0x24e8b63]
> 	base::Callback<>::Run() [0x46ebe3]
> 	MessageLoop::RunTask() [0xdeb64c]
> 	MessageLoop::DeferOrRunPendingTask() [0xdeb763]
> 	MessageLoop::DoWork() [0xdebf57]
> 	base::MessagePumpDefault::Run() [0xdf3dc0]
> 	MessageLoop::RunInternal() [0xdeb313]
> 	MessageLoop::RunHandler() [0xdeb1ca]
> 	MessageLoop::RunAllPending() [0xdeab48]
> 	fileapi::FileSystemTestOriginHelper::TearDown() [0x87bcda]
> 	ObfuscatedFileUtilTest::TearDown() [0x8cbbed]
> 	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1d40986]
> 	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1d3d9f8]
> 	testing::Test::Run() [0x1d325c9]
> 	testing::TestInfo::Run() [0x1d32da4]
> 	testing::TestCase::Run() [0x1d3349a]
> 	testing::internal::UnitTestImpl::RunAllTests() [0x1d38425]
> 	testing::internal::HandleSehExceptionsInMethodIfSupported<>() [0x1d417d8]
> 	testing::internal::HandleExceptionsInMethodIfSupported<>() [0x1d3e227]
> 	testing::UnitTest::Run() [0x1d36f0e]
> 	base::TestSuite::Run() [0xd45081]
> 	content::UnitTestTestSuite::Run() [0xb176cc]
> 	main [0x6e7e96]
> 	0x7fedd37dfc4d
> 	0x4293f9

Hmm...apparently there are some other problems, still.  Let me look into them
further.

[ericu, 2012-06-19 19:07:12]

Well, I've improved on this now [tests should pass], but I'm still not happy
with it.  I don't like doing a string comparison there, but isNull is returning
false even though the origin serializes to "null", which then produces an
invalid GURL with nonzero spec length.  Any thoughts?

[ericu, 2012-06-19 19:55:33]

On 2012/06/19 19:07:12, ericu wrote:
> Well, I've improved on this now [tests should pass], but I'm still not happy
> with it.  I don't like doing a string comparison there, but isNull is
returning
> false even though the origin serializes to "null", which then produces an
> invalid GURL with nonzero spec length.  Any thoughts?

OK, the reason for isNull acting that way is that it's only true if the
WebSecurityOrigin doesn't wrap a SecurityOrigin.  If it wraps anything at all,
including one that's unique [and thus stringifies to "null", such as one created
from an empty string] then isNull is false.  So the string check seems to be
correct.

[kinuko, 2012-06-20 11:39:01]

Thanks for digging this deeper!

https://chromiumcodereview.appspot.com/10572006/diff/4003/webkit/database/dat...
File webkit/database/database_util.cc (right):

https://chromiumcodereview.appspot.com/10572006/diff/4003/webkit/database/dat...
webkit/database/database_util.cc:84: if
(web_security_origin.toString().equals("null")) {
Looks like maybe we could instead call web_security_origin.isUnique()?

(From the comments in WebCore/page/SecurityOrigin.h and the code in
SecurityOrigin.cpp)

[ericu, 2012-06-20 22:25:41]

On 2012/06/20 11:39:01, kinuko wrote:
> Thanks for digging this deeper!
> 
>
https://chromiumcodereview.appspot.com/10572006/diff/4003/webkit/database/dat...
> File webkit/database/database_util.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10572006/diff/4003/webkit/database/dat...
> webkit/database/database_util.cc:84: if
> (web_security_origin.toString().equals("null")) {
> Looks like maybe we could instead call web_security_origin.isUnique()?
> 
> (From the comments in WebCore/page/SecurityOrigin.h and the code in
> SecurityOrigin.cpp)

Yes, I think you're right.  Done.

[kinuko, 2012-06-21 02:53:26]

lgtm

[ericu, 2012-07-09 19:37:44]

On 2012/06/21 02:53:26, kinuko wrote:
> lgtm

Added MichaelN for OWNERS approval.

[michaeln, 2012-07-09 21:20:02]

lgtm2

[commit-bot: I haz the power, 2012-07-09 21:21:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ericu@chromium.org/10572006/11001

[commit-bot: I haz the power, 2012-07-09 22:49:24]

Change committed as 145766