Closed Bug 1415793 Opened 2 years ago Closed 2 years ago

Fix failure of mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html relying on non-comformant Promise handling

Categories

(WebExtensions :: Android, defect, P5)

defect

Tracking

(firefox57 wontfix, firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: bevis, Assigned: bevis)

References

Details

Attachments

(1 file)

This is a follow-up of the try result in bug 1193394 comment 58:
(C2 in android 4.2 x86 build: https://treeherder.mozilla.org/logviewer.html#?job_id=143222809&repo=try&lineNumber=2385)

TEST-UNEXPECTED-FAIL | dom/svg/test/test_tabindex.html | The active element tabindex is 3 - got -1, expected 3
Also failed in Android 4.3 API16+ opt/debug build
Priority: -- → P5
The log in comment 0 is wrong.
It shall be:
FAIL | toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html | Test timed out.
FAIL | toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html | Extension left running at test shutdown.
in https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71a2448564552227b51ca4db12e12e242a50836&selectedJob=151674844
Sorry, it shall be this one:
FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | The extension popup tab should have been closed
got [object Object], expected null
FAIL | mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html | The extension popup tab should have been closed
got [object Object], expected null
in https://treeherder.mozilla.org/#/jobs?repo=try&revision=b71a2448564552227b51ca4db12e12e242a50836&selectedJob=151659780
The failure cause is that the implementation removes the tab after TabClose event is fired which looks weird from the event listener perspective:
https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1252,1286
and expects that the resolved callback only be called at the end of current task:
https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/test/mochitest/test_ext_activeTab_permission.html#327,456

See more explanation in bug 1193394 comment 58 for how Promise callbacks are expected to be done.
The update patches of bug 1193394 are available on the try result in comment 3.
Hi Nevin,

Could you help to check the symptom in comment 4?
The tab shall be invalid when checking from the event listener of "TabClose".
It looks strange to me if that BrowserApp.getTabForId(tabId) returns valid tab if it was called immediately from the "TabClose" event listener or from the promise callback if the solution in bug 1193394 is applied which runs the resolve callback immediately after event listener is returned.

However, if I try to do "this._tabs.splice(tabIndex, 1)" earlier before dispatching the event, the test will be timeout somehow in other place. :-(
Flags: needinfo?(cnevinchen)
Given the fact the SessionStore relies on the availability of the closed tab during "TabClose" event:
https://searchfox.org/mozilla-central/rev/ff462a7f6b4dd3c3fdc53c9bc0b328f42a5b3d2b/mobile/android/components/SessionStore.js#683

We should update the test case to verify the removal of the tab until next tick instead.
Flags: needinfo?(cnevinchen)
Due to the change of Promise scheduling as explained in bug 1193394 comment 58, to comply with HTML standard, it is possible to perform promise callbacks earlier after a event listener is returned.
In addition, according to the implementation of browser.js and SessionStore.js, it seems expected to keep the tab alive during the dispatching of the TabClose event. Hence, to verify the removal of the tab using promise, we should check until the next tick of the event loop instead.
Assignee: nobody → btseng
Attachment #8937935 - Flags: review?(lgreco)
(In reply to Bevis Tseng[:bevis][:btseng] from comment #7)
> Due to the change of Promise scheduling as explained in bug 1193394 comment 58,
Sorry, it's explained in bug 1193394 comment 48 instead.
Comment on attachment 8937935 [details] [diff] [review]
(v1) Patch: Check the removal of a tab until next tick.

Review of attachment 8937935 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the additional details related to the rationale and context related to this fix.

I also took a brief look at the other fixes applied to webextensions tests with similar issues (e.g. for Bug 1414170), and it sounds right to me that we use the same strategy to fix it in this test as well.
Attachment #8937935 - Flags: review?(lgreco) → review+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23b8e3ed86a8
Check the removal of a tab until next tick. r=rpl
https://hg.mozilla.org/mozilla-central/rev/23b8e3ed86a8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.