Potential infinite loop in WebTestWithWebState::LoadHtml |
|||||||
Issue description
In WebTestWithWebState::LoadHtml, at the end there is the following code:
if (![ExecuteJavaScript(@"0;") isEqual:@0]) {
LoadHtml(html, url);
}
This recursively call the current method with the same parameters. However, it is possible that the ExecuteJavaScript(...) fails also in the recursive invocation, leading to another call to WebTestWithWebState::LoadHtml. If the javascript execution continously fail, the call stack will grow larger and larger.
This can lead to huge call stack like the following (captured in a failing test, part of the stack omitted):
* thread #1, queue = 'com.apple.main-thread', stop reason = step over
frame #0: 0x000000010320fb49 ios_chrome_unittests`web::WebTestWithWebState::ExecuteJavaScript(this=0x00006180001fb300, script="0;") at web_test_with_web_state.mm:181
* frame #1: 0x000000010320f5f5 ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:131
frame #2: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #3: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #4: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
...
frame #992: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #993: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #994: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #995: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #996: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #997: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #998: 0x000000010320f6cf ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", url=0x00007ffeee64bfb8) at web_test_with_web_state.mm:132
frame #999: 0x00000001032100cc ios_chrome_unittests`web::WebTestWithWebState::LoadHtml(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>") at web_test_with_web_state.mm:138
frame #1000: 0x0000000101f0ee49 ios_chrome_unittests`TextToSpeechListenerTest::TestExtraction(this=0x00006180001fb300, html="<html><head><script>(function(){var _a_tts='dGVzdGF1ZG8zMm9pbw==';var _m_tts= {}})</script></head><body></body></html>", expected_audio_data=13 bytes) at text_to_speech_listener_unittest.mm:81
frame #1001: 0x0000000101f0ed34 ios_chrome_unittests`TextToSpeechListenerTest_ValidAudioDataTest_Test::TestBody(this=0x00006180001fb300) at text_to_speech_listener_unittest.mm:98
frame #1002: 0x0000000101fe226e ios_chrome_unittests`void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(object=0x00006180001fb300, method=21 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00, location="the test body")(), char const*) at gtest.cc:2395
frame #1003: 0x0000000101fbe372 ios_chrome_unittests`void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(object=0x00006180001fb300, method=21 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00, location="the test body")(), char const*) at gtest.cc:2448
frame #1004: 0x0000000101fbe2a6 ios_chrome_unittests`testing::Test::Run(this=0x00006180001fb300) at gtest.cc:2467
frame #1005: 0x0000000101fbf21d ios_chrome_unittests`testing::TestInfo::Run(this=0x00007f832ce8b9f0) at gtest.cc:2645
frame #1006: 0x0000000101fc052c ios_chrome_unittests`testing::TestCase::Run(this=0x00007f832ce8bb00) at gtest.cc:2763
frame #1007: 0x0000000101fcd0cb ios_chrome_unittests`testing::internal::UnitTestImpl::RunAllTests(this=0x00007f832ce016c0) at gtest.cc:4638
frame #1008: 0x0000000101fe501e ios_chrome_unittests`bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x00007f832ce016c0, method=30 cd fc 01 01 00 00 00 00 00 00 00 00 00 00 00, location="auxiliary test code (environments or event listeners)")(), char const*) at gtest.cc:2395
frame #1009: 0x0000000101fcccb2 ios_chrome_unittests`bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x00007f832ce016c0, method=30 cd fc 01 01 00 00 00 00 00 00 00 00 00 00 00, location="auxiliary test code (environments or event listeners)")(), char const*) at gtest.cc:2448
frame #1010: 0x0000000101fccba9 ios_chrome_unittests`testing::UnitTest::Run(this=0x0000000108230360) at gtest.cc:4250
frame #1011: 0x0000000103d76f61 ios_chrome_unittests`RUN_ALL_TESTS() at gtest.h:2297
frame #1012: 0x0000000103d7639e ios_chrome_unittests`base::TestSuite::Run(this=0x00007ffeee64f0c0) at test_suite.cc:272
frame #1013: 0x0000000103d7c8b1 ios_chrome_unittests`::-[ChromeUnitTestDelegate runTests](self=0x0000610000017610, _cmd="runTests") at test_support_ios.mm:157
frame #1014: 0x0000000111d02f35 Foundation`__NSFireDelayedPerform + 409
frame #1015: 0x000000010eb4b174 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
frame #1016: 0x000000010eb4ae32 CoreFoundation`__CFRunLoopDoTimer + 1026
frame #1017: 0x000000010eb4a9ea CoreFoundation`__CFRunLoopDoTimers + 266
frame #1018: 0x000000010eb42404 CoreFoundation`__CFRunLoopRun + 2308
frame #1019: 0x000000010eb41889 CoreFoundation`CFRunLoopRunSpecific + 409
frame #1020: 0x0000000116d659c6 GraphicsServices`GSEventRunModal + 62
frame #1021: 0x000000010f1ad5d6 UIKit`UIApplicationMain + 159
frame #1022: 0x0000000103d7cbf2 ios_chrome_unittests`base::RunTestsFromIOSApp() at test_support_ios.mm:213
frame #1023: 0x0000000103d7626f ios_chrome_unittests`base::TestSuite::Run(this=0x00007ffeee64f0c0) at test_suite.cc:253
frame #1024: 0x00000001015b303d ios_chrome_unittests`int base::internal::FunctorTraits<int (base::TestSuite::*)(), void>::Invoke<IOSChromeUnitTestSuite*>(method=50 62 d7 03 01 00 00 00 00 00 00 00 00 00 00 00, receiver_ptr=0x00007ffeee64e9f0)(), IOSChromeUnitTestSuite*&&) at bind_internal.h:447
frame #1025: 0x00000001015b2f84 ios_chrome_unittests`int base::internal::InvokeHelper<false, int>::MakeItSo<int (functor=0x000061c00026d360, args=0x00007ffeee64e9f0)(), IOSChromeUnitTestSuite*>(int (base::TestSuite::* const&&&)(), IOSChromeUnitTestSuite*&&) at bind_internal.h:530
frame #1026: 0x00000001015b2f35 ios_chrome_unittests`int base::internal::Invoker<base::internal::BindState<int (base::TestSuite::*)(), base::internal::UnretainedWrapper<IOSChromeUnitTestSuite> >, int ()>::RunImpl<int (functor=0x000061c00026d360, bound=0x000061c00026d370, (null)=std::__1::index_sequence<0UL> @ 0x00007ffeee64ea08)(), std::__1::tuple<base::internal::UnretainedWrapper<IOSChromeUnitTestSuite> > const&, 0ul>(int (base::TestSuite::* const&&&)(), std::__1::tuple<base::internal::UnretainedWrapper<IOSChromeUnitTestSuite> > const&&&, std::__1::integer_sequence<unsigned long, 0ul>) at bind_internal.h:604
frame #1027: 0x00000001015b2e4c ios_chrome_unittests`base::internal::Invoker<base::internal::BindState<int (base::TestSuite::*)(), base::internal::UnretainedWrapper<IOSChromeUnitTestSuite> >, int ()>::Run(base=0x000061c00026d340) at bind_internal.h:586
frame #1028: 0x0000000103d7ed3d ios_chrome_unittests`base::RepeatingCallback<int ()>::Run(this=0x00007ffeee64eeb0) const & at callback.h:124
frame #1029: 0x0000000103d7ecbd ios_chrome_unittests`base::LaunchUnitTests(argc=2, argv=0x00007ffeee64f168, run_test_suite=0x00007ffeee64eeb0)> const&) at unit_test_launcher_ios.cc:39
frame #1030: 0x00000001015b1942 ios_chrome_unittests`main(argc=2, argv=0x00007ffeee64f168) at run_all_unittests.cc:24
frame #1031: 0x000000011812ed81 libdyld.dylib`start + 1
frame #1032: 0x000000011812ed81 libdyld.dylib`start + 1
This should not happen in practice if the test data is correct. But I think that badly written tests should not lead to stack overflow. Maybe the method should receive an extra parameter corresponding to the try count and abort after a fixed number of failed attempts.
,
Feb 22 2018
I agree that a poorly written test should not be able to crash the app. Instead WebTestWithWebState::LoadHtml should return bool indicating success (crbug.com/780062). I would really like to fix this, but I will not have bandwidth for quite a while. I do plan to refactor ios/web testing code in 2018 and this bug can definitely wait.
,
Feb 23 2018
For the record, this causes the following tests to always fails on my machine when running ios_chrome_unittests on simulator representing iPhone 8 Plus running iOS 11 in Xcode 9.2: [ FAILED ] SyncCreatePassphraseCollectionViewControllerTest.TestOnStateChanged [ FAILED ] SyncEncryptionPassphraseCollectionViewControllerTest.TestOnStateChangedCorrectPassphrase [ FAILED ] StaticHtmlViewControllerTest.L10NTest [ FAILED ] TextToSpeechListenerTest.ValidAudioDataTest [ FAILED ] ExternalAppLaunchingStateTest.TestUpdateWithLaunchRequest
,
Feb 23 2018
Sylvain, does crrev.com/c/934628 fixes your failures. I think crrev.com/c/934628 is the right direction anyway, and I will try to land it.
,
Feb 23 2018
BTW, I found that this is also happening on the CQ, but the test appear green. For example https://ci.chromium.org/buildbot/tryserver.chromium.mac/ios-simulator/401854 ios_chrome_unittests is green but the test mentionned in #3 are failing with the same message in the log as I can see in my local build. Note that there are other failures than just the infinite recursion, for example: [ RUN ] SyncCreatePassphraseCollectionViewControllerTest.TestOnStateChanged ../../ios/chrome/browser/ui/settings/sync_create_passphrase_collection_view_controller_unittest.mm:192: Failure Value of: testing::WaitUntilConditionOrTimeout( testing::kWaitForUIElementTimeout, ^bool() { return [nav_controller_ topViewController] != sync_controller; }) Actual: false Expected: true [ FAILED ] SyncCreatePassphraseCollectionViewControllerTest.TestOnStateChanged (17 ms) Here the test failed with a single error. So I'm wondering whether there is an issue with testing::WaitUntilConditionOrTimeout(). AFAICT, it looks like this no longer spin the MessageLoop. Could this be the cause of those intermittent failures?
,
Feb 23 2018
Also, for #5, if SyncCreatePassphraseCollectionViewControllerTest.TestOnStateChanged fails, then I get the infinite recursion. If it succeed, I don't. So the fix in crrev.com/c/934628 may just cause the test to time out earlier.
,
Feb 23 2018
Further investigation makes me think that the recursive behaviour is just a collateral damage of a change in the scheduling of tasks. Creating a separate issue for investigating the timeouts of testing::WaitUntilConditionOrTimeout. => https://bugs.chromium.org/p/chromium/issues/detail?id=815189 for the investigation.
,
Feb 26 2018
So in 815189 we identified that the loading failed due to a stray mock of NSDate. With the underlying issue fixed, the loading no longer fails, and we are no longer experiencing the deeply recursive callstacks. https://chromium-review.googlesource.com/c/chromium/src/+/934628 still looks good, so I think we should land it nonetheless (there is no reason to have this potentially recursive call). I've +1-ed the CL.
,
Feb 26 2018
,
Feb 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/348ccc5791f935e8643fde17bf874fb7aefff1ad commit 348ccc5791f935e8643fde17bf874fb7aefff1ad Author: Eugene But <eugenebut@chromium.org> Date: Mon Feb 26 16:14:26 2018 Prevent endless recursion in WebTestWithWebState::LoadHtml. Instead of recursively reloading html, this function now waits until the script execution is possible. Bug: 814709 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I45a6bc7413d2a508aaa4b6af123c243f96b8bc42 Reviewed-on: https://chromium-review.googlesource.com/934628 Reviewed-by: Sylvain Defresne <sdefresne@chromium.org> Commit-Queue: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/heads/master@{#539151} [modify] https://crrev.com/348ccc5791f935e8643fde17bf874fb7aefff1ad/ios/web/public/test/web_test_with_web_state.mm
,
Feb 26 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by sdefresne@chromium.org
, Feb 22 2018