Eliminate silently letting errors pass on pak loading.

Eliminate silently letting errors pass on pak loading.

BUG=410411
TEST=no visible change

Committed: https://crrev.com/03c8c9d936594163d43222a3da0268b3dd9af656
Cr-Commit-Position: refs/heads/master@{#299717}

[Avi (use Gerrit), 2014-09-03 19:17:29]

avi@chromium.org changed reviewers:
+ tfarina@chromium.org, tony@chromium.org

[Avi (use Gerrit), 2014-09-03 19:17:30]

Tony for review, Thiago FYI.

I can't imagine that this will go this cleanly. I want to see what the
preliminary try run looks like. Thiago, your patch will have to land first :)

[tony, 2014-09-03 19:19:56]

Code change LGTM.

[tfarina, 2014-09-03 19:22:41]

Avi, thanks for taking time to look deeply at this! It was very appreciate your
help on this. ;)

Please, push https://codereview.chromium.org/521473002/ to CQ if it looks OK to
you.

[tfarina, 2014-09-03 19:23:52]

lgtm (rubber-stamp)

[Avi (use Gerrit), 2014-09-04 22:45:33]

avi@chromium.org changed reviewers:
+ bengr@chromium.org

[Avi (use Gerrit), 2014-09-04 22:45:34]

bengr: Can you review dom_distiller? The resources don't need to be loaded for
the unittests, only the browsertests.
tony: Re-review? This is what it actually takes to work :)

[tony, 2014-09-04 22:55:48]

https://codereview.chromium.org/537063002/diff/160001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_unittest.cc (right):

https://codereview.chromium.org/537063002/diff/160001/ui/base/resource/resour...
ui/base/resource/resource_bundle_unittest.cc:189:
.WillOnce(Return(base::FilePath()));
Why did this change?

[Avi (use Gerrit), 2014-09-04 23:02:51]

https://codereview.chromium.org/537063002/diff/160001/ui/base/resource/resour...
File ui/base/resource/resource_bundle_unittest.cc (right):

https://codereview.chromium.org/537063002/diff/160001/ui/base/resource/resour...
ui/base/resource/resource_bundle_unittest.cc:189:
.WillOnce(Return(base::FilePath()));
On 2014/09/04 22:55:48, tony wrote:
> Why did this change?

This unit test is testing that the delegate will get the desired callback. So it
makes a call to AddDataPackFromPath with a completely bogus path. If the
delegate returns the bogus path, like it once did, then the resource bundle will
actually try to load that bogus pak file and die.

It's documented that returning an empty file path from the delegate call
GetPathForResourcePack will cancel the load. And once the
MockResourceBundleDelegate gets the call to GetPathForResourcePack we have
achieved the point of the unit test. So we cancel the load before it crashes.

[tony, 2014-09-04 23:12:37]

Thanks for the explanation.  LGTM.

[bengr, 2014-09-05 00:59:57]

lgtm with nit.

https://codereview.chromium.org/537063002/diff/160001/components/dom_distille...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://codereview.chromium.org/537063002/diff/160001/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:15: #include
"base/path_service.h"
Do you still need this?

[Avi (use Gerrit), 2014-09-05 15:17:27]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2014-09-05 15:17:42]

https://codereview.chromium.org/537063002/diff/160001/components/dom_distille...
File components/dom_distiller/core/distiller_unittest.cc (right):

https://codereview.chromium.org/537063002/diff/160001/components/dom_distille...
components/dom_distiller/core/distiller_unittest.cc:15: #include
"base/path_service.h"
On 2014/09/05 00:59:56, bengr1 wrote:
> Do you still need this?

Probably not. Removed.

[commit-bot: I haz the power, 2014-09-05 15:18:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/537063002/180001

[commit-bot: I haz the power, 2014-09-05 16:04:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:04:25]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2014-09-05 16:07:21]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-09-05 16:07:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/537063002/180001

[commit-bot: I haz the power, 2014-09-05 18:12:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:12:37]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Avi (use Gerrit), 2014-09-05 18:35:40]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-09-05 18:37:39]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/avi@chromium.org/537063002/180001

[commit-bot: I haz the power, 2014-09-05 20:51:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 20:51:39]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[tfarina, 2014-09-09 15:32:20]

Avi, is android_dbg_tests_recipe just flaky? Let's put it on CQ again to see?

Let me know if I can help.

[Avi (use Gerrit), 2014-09-09 16:10:19]

On 2014/09/09 15:32:20, tfarina wrote:
> Avi, is android_dbg_tests_recipe just flaky? Let's put it on CQ again to see?
> 
> Let me know if I can help.

I'm 99% sure it's not flaky. But I don't have an Android setup to extract out a
stack trace to figure out what's going on.

Do you have an Android setup? If so, it would be awesome if you could take a
look at this.

[Avi (use Gerrit), 2014-10-03 01:46:02]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-10-03 01:47:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/537063002/240001

[Avi (use Gerrit), 2014-10-03 02:26:27]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2014-10-04 17:43:44]

avi@chromium.org changed reviewers:
+ caitkp@chromium.org

[Avi (use Gerrit), 2014-10-04 17:43:45]

Cait, can you take a look at the two components files? They were crafted by
Newton, an Android expert.

[Cait (Slow), 2014-10-06 15:37:16]

components/ LGTM

[tony, 2014-10-15 17:35:21]

LGTM

https://codereview.chromium.org/537063002/diff/340001/ui/base/ui_base_tests.gypi
File ui/base/ui_base_tests.gypi (right):

https://codereview.chromium.org/537063002/diff/340001/ui/base/ui_base_tests.g...
ui/base/ui_base_tests.gypi:109: 'cp',
I thought there was a 'copies' command that gyp knows about.  Maybe that doesn't
run at the right time?

[Avi (use Gerrit), 2014-10-15 17:39:59]

https://codereview.chromium.org/537063002/diff/340001/ui/base/ui_base_tests.gypi
File ui/base/ui_base_tests.gypi (right):

https://codereview.chromium.org/537063002/diff/340001/ui/base/ui_base_tests.g...
ui/base/ui_base_tests.gypi:109: 'cp',
On 2014/10/15 17:35:21, tony wrote:
> I thought there was a 'copies' command that gyp knows about.  Maybe that
doesn't
> run at the right time?

I don't know; I don't speak gyp very well. I tried using ln like in the mac
gypi, but it failed. cp was the only thing that worked here.

[Avi (use Gerrit), 2014-10-15 17:57:43]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2014-10-15 17:58:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/537063002/340001

[commit-bot: I haz the power, 2014-10-15 18:14:30]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-10-15 18:15:23]

Patchset 18 (id:??) landed as
https://crrev.com/03c8c9d936594163d43222a3da0268b3dd9af656
Cr-Commit-Position: refs/heads/master@{#299717}

[dcheng, 2014-10-16 00:30:30]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2014-10-16 00:30:31]

I think this may have exploded the waterfall. content_browsertests is very sad:
@@@STEP_LOG_LINE@StreamToFile@URLRequestInfoTest.StreamToFile (run #1):@@@
@@@STEP_LOG_LINE@StreamToFile@[ RUN      ] URLRequestInfoTest.StreamToFile@@@
@@@STEP_LOG_LINE@StreamToFile@[2672:860:1015/122308:7989263:ERROR:memory_mapped_file.cc(41)]
Couldn't open
e:\b\swarm_slave\work\run_tha_test1k2xmg\out\Debug\chrome_100_percent.pak@@@
@@@STEP_LOG_LINE@StreamToFile@[2672:860:1015/122308:7989263:ERROR:data_pack.cc(78)]
Failed to mmap datapack@@@
@@@STEP_LOG_LINE@StreamToFile@[2672:860:1015/122308:7989263:FATAL:resource_bundle.cc(674)]
Failed to load
e:\b\swarm_slave\work\run_tha_test1k2xmg\out\Debug\chrome_100_percent.pak@@@
@@@STEP_LOG_LINE@StreamToFile@Backtrace:@@@
@@@STEP_LOG_LINE@StreamToFile@	base::operator\u003C\u003C [0x013384F1+533056]@@@
@@@STEP_LOG_LINE@StreamToFile@	base::operator\u003C\u003C
[0x013C7E8F+1121246]@@@
@@@STEP_LOG_LINE@StreamToFile@	ui::PlatformAccelerator::~PlatformAccelerator
[0x049B1A48+756549]@@@
@@@STEP_LOG_LINE@StreamToFile@	ui::PlatformAccelerator::~PlatformAccelerator
[0x049B1750+755789]@@@
@@@STEP_LOG_LINE@StreamToFile@	ui::PlatformAccelerator::~PlatformAccelerator
[0x049BD7E4+805089]@@@
@@@STEP_LOG_LINE@StreamToFile@	ui::PlatformAccelerator::~PlatformAccelerator
[0x049B409D+766362]@@@
@@@STEP_LOG_LINE@StreamToFile@	content::RenderViewTest::SetUp
[0x007EEE94+900]@@@
@@@STEP_LOG_LINE@StreamToFile@	content::URLRequestInfoTest::SetUp
[0x006D57CE+62]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::internal::HandleExceptionsInMethodIfSupported\u003Ctesting::Test,void>
[0x008A71A7+55]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::Test::Run [0x008C509B+107]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::TestInfo::Run [0x008C537D+221]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::TestCase::Run [0x008C521F+239]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::internal::UnitTestImpl::RunAllTests
[0x008C5856+726]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::internal::HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl,bool>
[0x008A7297+55]@@@
@@@STEP_LOG_LINE@StreamToFile@	testing::UnitTest::Run [0x008C550B+267]@@@
@@@STEP_LOG_LINE@StreamToFile@	RUN_ALL_TESTS [0x009B133F+15]@@@
@@@STEP_LOG_LINE@StreamToFile@	base::TestSuite::Run [0x009B1553+179]@@@
@@@STEP_LOG_LINE@StreamToFile@	content::ContentTestLauncherDelegate::RunTestSuite
[0x008FDF5C+60]@@@
@@@STEP_LOG_LINE@StreamToFile@	content::LaunchTests [0x007F95F7+967]@@@
@@@STEP_LOG_LINE@StreamToFile@	main [0x008FE001+113]@@@
@@@STEP_LOG_LINE@StreamToFile@	__tmainCRTStartup [0x008FBBD9+409]
(f:\dd\vctools\crt\crtw32\dllstuff\crtexe.c:626)@@@
@@@STEP_LOG_LINE@StreamToFile@	mainCRTStartup [0x008FBD1D+13]
(f:\dd\vctools\crt\crtw32\dllstuff\crtexe.c:466)@@@
@@@STEP_LOG_LINE@StreamToFile@	BaseThreadInitThunk [0x7584338A+18]@@@
@@@STEP_LOG_LINE@StreamToFile@	RtlInitializeExceptionChain [0x77049F72+99]@@@
@@@STEP_LOG_LINE@StreamToFile@	RtlInitializeExceptionChain [0x77049F45+54]@@@
@@@STEP_LOG_LINE@StreamToFile@@@@

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%...
etc

[dcheng, 2014-10-16 00:30:58]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/661503003/ by dcheng@chromium.org.

The reason for reverting is: Causing content_browsertests failures due to pak
loading failure..

[Zhenyao Mo, 2014-10-18 03:23:49]

A revert of this CL (patchset #18 id:340001) has been created in
https://codereview.chromium.org/652623005/ by zmo@chromium.org.

The reason for reverting is: Cause issue
https://code.google.com/p/chromium/issues/detail?id=424024, mostly confirmed on
Mac Debug (Intel) bot, but could be responsible for windows bot issue too. 
We'll see..