1505670 - Target tab may already be destroyed while waiting for the next microtask checkpoint in ext-browser.js

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/browser/components/extensions/parent/ext-browser.js#482-484
> class TabTracker extends TabTrackerBase {
> ...
>   handleEvent(event) {
>     let nativeTab = event.target;
> 
>     switch (event.type) {
> ...
>       case "TabSelect":
>         // Because we are delaying calling emitCreated above, we also need to
>         // delay sending this event because it shouldn't fire before onCreated.
>         Promise.resolve().then(() => {
>           this.emitActivated(nativeTab);
>         });
>         break;

It may fail if the tab is destroyed before the resolution handler is called.
which results in hitting the following:

https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/browser/components/extensions/parent/ext-browser.js#337
> class TabTracker extends TabTrackerBase {
> ...
>   setId(nativeTab, id) {
>     if (!nativeTab.parentNode) {
>       throw new Error("Cannot attach ID to a destroyed tab.");

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Check if the tab is still alive after waiting for the next microtask checkpoint, in TabTracker#handleEvent.

A lot of test fails because of this after fixing bug 1498775.
(thus this patch has no testcase)

Comment 2

[Tooru Fujisawa [:arai]]

Comment on attachment 9023519 [details] [diff] [review]
Check if the tab is still alive after waiting for the next microtask checkpoint, in TabTracker#handleEvent.

oops, wrong patch

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Check if the tab is still alive after waiting for the next microtask checkpoint, in TabTracker#handleEvent.

here's the correct one that checks the tab's existence, which is passed to the methods.

Comment 4

[Shane Caraveo (:mixedpuppy)]

The patch looks reasonable.  I'd like to see the test failures so I can see what tests are failing and how they are failing.  Can you run a try for bug 1498775?

Comment 5

[Tooru Fujisawa [:arai]]

here's the try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=441a0f5f355137bfc230f384a9a27b0785d5256c&selectedJob=210260814

most M(bc) jobs failed there are due to this.

and this is try with this patch and bug 1505667 patch applid
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dcd08bde30eb73cf21ff93b0403f740a5eea4e6

remaining failures are for bug 1505701 and bug 1505707.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9023524 [details] [diff] [review]
Check if the tab is still alive after waiting for the next microtask checkpoint, in TabTracker#handleEvent.

Thanks!

Comment 7

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/73042df54a94e6b961e5f7128d15a5e4ff01c56b
Bug 1505670 - Check if the tab is still alive after waiting for the next microtask checkpoint, in TabTracker#handleEvent. r=mixedpuppy

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/73042df54a94

Comment 9

[Vlad Jiman]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag. Thanks

Comment 10

[Tooru Fujisawa [:arai]]

This is tested once bug 1498775 gets fixed (I hope shortly), and also is already covered in existing testcases.
I think manual test is not necessary.