This is a follow-up of the try result in bug 1193394 comment 58: (C2 in android build: https://treeherder.mozilla.org/logviewer.html#?job_id=143219758&repo=try&lineNumber=2796) 253 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_form_data.html | outer value restored correctly - got "", expected "browser_formdata_0.9756034525331857" 254 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_form_data.html | inner value restored correctly - got "", expected "browser_formdata_0.34039943315999643" 263 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_form_data.html | can go back - got false, expected true

May I know who's the owner of this?

I filed this bug according to which module this test case was created in. There are multiple reasons that cause this failure. It could be the abuse of promise in the test case(bug 1414136), the implementation problem in front-end(bug 1416153) or inside the DOM API(bug 1413125, bug 1413466) according to the issue I've fixed or analyzed so far. You can take a look at the slide and explanation in bug 1193394 comment 48 to see what's the impact when bug 1193394 is fixed.

Take this bug first to follow up.

There seems more test failures that was discovered if test_session_form_data.html is fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b614338a3de71a1d8dd5e0dbfb3f5353c72a9b86&selectedJob=154676974 Maybe I could fix the session store related ones in this bug and file more bugs to follow-up other failures. 196 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_parentid.html | Test timed out. 214 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_scroll_position.html | scrollY restored correctly - got +0, expected 233 238 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_scroll_position.html | Test timed out. 244 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_session_zombification.html | TypeError: BrowserApp.selectedTab is null - Should not throw any errors 253 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | TypeError: BrowserApp.selectedTab is null - Should not throw any errors 264 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_video_discovery.html | uncaught exception - TypeError: BrowserApp.selectedTab is null at setup_browser@chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_video_discovery.html:71:56 297 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | Test timed out. 298 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | Extension left running at test shutdown 299 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_unrecognizedprop_warning.html | Test left extra windows or tabs: {"extraWindows":[],"extraTabs":["http://mochi.test:8888/chrome/toolkit/components/extensions/test/mochitest/file_sample.html"]} 370 INFO TEST-UNEXPECTED-FAIL | toolkit/components/windowcreator/test/test_bug1170334_wbp_xmlstyle.html | Test timed out. 372 INFO TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | Skipping 9 remaining tests. 373 INFO TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | chrome://mochitests/content/chrome/toolkit/components/windowcreator/test/test_bug1170334_wbp_xmlstyle.html - finished in a non-clean fashion, probably because it didn't call SimpleTest.finish() 374 INFO TEST-UNEXPECTED-ERROR | (SimpleTest/TestRunner.js) | Finished in 319671ms 1426565Intermittent (SimpleTest/TestRunner.js) | 4 test timeouts, giving up. - expected PASS 1412314Intermittent (SimpleTest/TestRunner.js) | /tests/dom/canvas/test/webgl-conf/generated/test_2_conformance2__textures__video__tex-3d-rgba16f-rgba-float.html - finished in a non-clean fashion, probably because it didn't call SimpleTest.finish()

As explained in bug 1193394 comment 48, to meet the HTML spec , the resolved/rejected Rromise callbacks should be performed earlier at the returned of a js callback/event listener if possible instead of at the end of current task. In these SessionStore test cases, with the fix of bug 1193394 applied, the promise callbacks will be invoked immediately when the calling of a event listener is returned. However, 1. For "SSTabCloseProcessed", the root cause is the same to bug 1415793 comment 6 that there is still some change to be done after the event dispatching and before the test to be continued, so we should resume the test at next event tick instead. 2. For "load" event, the root cause is the same to bug 1414136 comment 2, if load a new URI at the end of "load" event dispatching it will be identified as an redirection of current URI to the new URI and the current session history entry will be replaced by the new URI instead of adding a new entry into the session history, so we should also resume the test at next event tick instead. Try result of with/without fix of bug 1193394 looks good, btw: 1. (with) https://treeherder.mozilla.org/#/jobs?repo=try&revision=473879e8f511a98b7c4a3b1553b802043ff9f9c9&group_state=expanded&selectedJob=154692956 2. (without) https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bd6e04a5f772a610c0f3dc1ca2f43aa31c83c70&group_state=expanded&selectedJob=154693083

Comment on attachment 8940667 [details] [diff] [review] (v1) Patch: Resolve "SSTabCloseProcessed" and "load" events at next tick to simluate user interactions properly with promise. Review of attachment 8940667 [details] [diff] [review]: ----------------------------------------------------------------- Stealing this from Michael, since for Fennec he's dealing with Activity Stream only anyway. For "SSTabClosedProcessed" I've eventually found the underlying issue: We get the index of the tab to be closed [1] because we need it for dispatching the TabClose event. Then we send the event [2] and because of the changed Promise handling, the test code now calls undoCloseTab from within the TabClose handler. When _handleTabClosed eventually resumes execution, we remove the tab [3] using the cached tabIndex value retrieved at [1] and end up removing the wrong tab, because between [1] and [3] the call to undoCloseTab modified the tabs array and inserted the restored tab at the index of the tab we actually wanted to delete. Outside of these tests I don't think we have any code that does anything that modifies the tabs array from within a TabClose handler and webextensions should be safe as well because the corresponding notification does a round trip to the event loop as well [4], but it would still be nicer to fix this. Without a larger refactoring of our tab event handling however I think the only easy solution is to call "tabIndex = this._tabs.indexOf(aTab);" again just before finally removing the tab (and a short comment explaining why), even if it seems a bit silly getting the tabIndex twice within such a short timespan. The "load" event handling changes seem fine, I'll trust your judgment on that. In any case thanks for looking at this. [1] https://dxr.mozilla.org/mozilla-central/rev/739484451a6399c7f156a0d960335606aa6c1221/mobile/android/chrome/content/browser.js#1246 [2] https://dxr.mozilla.org/mozilla-central/rev/739484451a6399c7f156a0d960335606aa6c1221/mobile/android/chrome/content/browser.js#1248 [3] https://dxr.mozilla.org/mozilla-central/rev/739484451a6399c7f156a0d960335606aa6c1221/mobile/android/chrome/content/browser.js#1283 [4] https://dxr.mozilla.org/mozilla-central/rev/739484451a6399c7f156a0d960335606aa6c1221/mobile/android/components/extensions/ext-utils.js#434-435

(In reply to Jan Henning [:JanH] from comment #6) > For "SSTabClosedProcessed" I've eventually found the underlying issue: > We get the index of the tab to be closed [1] because we need it for > dispatching the TabClose event. Then we send the event [2] and because of > the changed Promise handling, the test code now calls undoCloseTab from > within the TabClose handler. When _handleTabClosed eventually resumes > execution, we remove the tab [3] using the cached tabIndex value retrieved > at [1] and end up removing the wrong tab, because between [1] and [3] the > call to undoCloseTab modified the tabs array and inserted the restored tab > at the index of the tab we actually wanted to delete. Thanks for pointing out the problem with more details. > Without a larger refactoring of our tab event handling however I think the > only easy solution is to call "tabIndex = this._tabs.indexOf(aTab);" again > just before finally removing the tab (and a short comment explaining why), > even if it seems a bit silly getting the tabIndex twice within such a short > timespan. This makes more sense and makes _handleTabClosed() more robust!

1. Improve BrowserApp._handleTabClosed() to remove Tab correctly if new tab is added during "TabClose" dispatching. 2. Resolve "load" event in next event tick to simulate visiting a different page properly, so the session history will be added correctly. Try result of with/without fix of bug 1193394 looks good, btw: 1. (with) https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7edcda3f029b49a80f77612510c5db84701400a&group_state=expanded&selectedJob=154924122 2. (without) https://treeherder.mozilla.org/#/jobs?repo=try&revision=badd8bf4d3d87201599e039e3b8f979055a9da02&group_state=expanded&selectedJob=154924062

Comment on attachment 8940951 [details] [diff] [review] (v2) Patch: Fix test failures of SessionStore if comformant Promise handling is applied. Review of attachment 8940951 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, thanks.

Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/65a61d9e4309 Fix test failures of SessionStore if comformant Promise handling is applied. r=JanH