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

RESOLVED FIXED in Firefox 65

Status

P2
normal
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

unspecified
mozilla65
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 months ago
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.");
(Assignee)

Comment 1

4 months ago
A lot of test fails because of this after fixing bug 1498775.
(thus this patch has no testcase)
Attachment #9023519 - Flags: review?(mixedpuppy)
(Assignee)

Comment 2

4 months ago
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
Attachment #9023519 - Attachment is obsolete: true
Attachment #9023519 - Flags: review?(mixedpuppy)
(Assignee)

Comment 3

4 months ago
here's the correct one that checks the tab's existence, which is passed to the methods.
Attachment #9023524 - Flags: review?(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?
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 5

4 months ago
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.
Flags: needinfo?(arai.unmht)
Priority: -- → P2
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!
Attachment #9023524 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 7

4 months ago
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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/73042df54a94
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65

Comment 9

4 months ago
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
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 10

4 months ago
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.
Flags: needinfo?(arai.unmht) → qe-verify-
You need to log in before you can comment on or make changes to this bug.