Closed Bug 1391218 Opened 3 years ago Closed 2 years ago

Browser console error thrown when closing the last tab from a FF window in a multi FF window enviroment

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: aflorinescu, Assigned: rpl)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Enviroment:]
Windows 10 x64, Ubuntu 16.04, Mac OSX 10.12
55.0a1 20170602192203
57.0a1 20170816100153


[Description:]
I see the error in browser console when having multiple windows opened, each containing several tabs: then you close all the tabs from that window, and after the last tab from the window, the error will pop.

[Steps:]
1. Open FF and browser console.
2. Open several tabs.
3. Drag one tab out -> new window created.
4. In the new window, open several tabs.
5. Open several tabs in the new window (the window from step 3)
6. Close all the tabs from the new window.


[Actual Result:]
TypeError: parentDocShell.getDocShellEnumerator is not a function. 
(view-source:resource://devtools/server/actors/tab.js)

[Note:]
Initially I thought this is caused by photon work, in particular bug 1355426, but I reproduced this bug on Nightly 55, so i assume there is other cause.
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 
Firefox: 57.0a1, Build ID 	20170817100132

I have tested this issue on latest Firefox release (55.0.2) and latest Nightly (57.0a1) build. I have managed to reproduce it using the steps provided in the description. Considering this, using the Mozregression tool, I have managed to find a regression window. From the pushlog, it seems that the bug 1302702  has caused this.

Here are the results:
Last good revision: af3b8957b02d027ecf69b0a946d414ff0a91b96c
First bad revision: f0df4a7f6551e886388c876f0da8b2c1de86f64f
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=af3b8957b02d027ecf69b0a946d414ff0a91b96c&tochange=f0df4a7f6551e886388c876f0da8b2c1de86f64f

Luca, can you please take a look at this?
Flags: needinfo?(lgreco)
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Flags: needinfo?(lgreco)
Thanks for identifying the actual STR, I digged into it and the helper function which uses the `getDocShellEnumerator` function in the tab actor:

- http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/devtools/server/actors/tab.js#62

it is called for a docShell instance that is coming from the "chrome-webnavigation-destroy" observer registered by the chrome actor:

- http://searchfox.org/mozilla-central/rev/b258e6864ee3e809d40982bc5d0d5aff66a20780/devtools/server/actors/chrome.js#111

it looks that while in the "chrome-webnavigation-create" branch of that if statement a `subject.QueryInterface(Ci.nsIDocShell)` is called before passing it to `this._docShellCreated(subject)`, in the "chrome-webnavigation-destroy" branch (which calls `this._docShellDestroyed(subject)`) is not.

I've reproduced the issue locally and then I changed it to also call `subject.QueryInterface(Ci.nsIDocShell)` before passing the subject to the `_docShellDestroyed` method and this seems to be enough to fix the issue (the `getDocShellEnumerator` function is defined as expected).

I've attached a patch which applies the changes described above and pushed it to try (to be sure that it doesn't actually introduce any issue in any of the existent tests):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=1486d45854a0f49a4fee82bffad2a43d0269e53f
Attachment #8899556 - Flags: review?(poirot.alex)
Comment on attachment 8899556 [details]
Bug 1391218 - Fix 'getDocShellEnumerator is not a function' exception when closing the last tab in a window.

https://reviewboard.mozilla.org/r/170864/#review176202

Thanks!

::: devtools/server/actors/chrome.js:111
(Diff revision 1)
>    }
>    if (topic == "chrome-webnavigation-create") {
>      subject.QueryInterface(Ci.nsIDocShell);
>      this._onDocShellCreated(subject);
>    } else if (topic == "chrome-webnavigation-destroy") {
> +    subject.QueryInterface(Ci.nsIDocShell);

I imagine you could factorize that call by moving it to line 106/107. (same for tab.js)
Attachment #8899556 - Flags: review?(poirot.alex) → review+
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/2be1fd327e63
Fix 'getDocShellEnumerator is not a function' exception when closing the last tab in a window. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/2be1fd327e63
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Given that 56 is the current DevEdition, please nominate this for Beta approval when you get a chance.
Flags: needinfo?(lgreco)
Comment on attachment 8899556 [details]
Bug 1391218 - Fix 'getDocShellEnumerator is not a function' exception when closing the last tab in a window.

Approval Request Comment
[Feature/Bug causing the regression]: 
Bug 1302702 (changes related to "oop extension" on chrome and tabs remove debugging actors)

[User impact if declined]: 
a "TypeError: parentDocShell.getDocShellEnumerator is not a function" exception will be logged in the browser console every time the user closes the last tab in a window, it is not currently clear if there are other side-effects that would impact on the user.

[Is this code covered by automated tests?]:
No (there are multiple tests that test the chrome and tabs actor, but there is no explicit tests that checks that no unexpected exception are raised when closing a tab)

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, it can be verified using the STR in Comment 0.

[List of other uplifts needed for the feature/fix]: 
None

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
The change is small and restricted to the observer that the DevTools tab and chrome actors subscribes to watch for the created and destroyed docShells, and ensures that `subject.QueryInterface(Ci.nsIDocShell);` 
is also called for a destroyed docShell, while currently it is called only for a newly created docShell.

[String changes made/needed]: 
None
Flags: needinfo?(lgreco)
Attachment #8899556 - Flags: approval-mozilla-beta?
Hi Adrian,
Can you help check if the issue was fixed in the latest nightly?
Flags: qe-verify+
Flags: needinfo?(adrian.florinescu)
Verified as fixed:
Ubuntu 16.04, OS X : 57.0a1 20170828100127
Flags: needinfo?(adrian.florinescu)
Comment on attachment 8899556 [details]
Bug 1391218 - Fix 'getDocShellEnumerator is not a function' exception when closing the last tab in a window.

Fix a regression and was verified. Beta56+.
Attachment #8899556 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I reproduced this issue using Fx 55.0a1, build ID: 20170817100132, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 56.0b10, on Windows 10 x64, Ubuntu 14.04 LTS and mac OS X 10.12.6.

Cheers!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.