927630 - Hang if tabs switched while Inspector is opening.

Status: RESOLVED DUPLICATE of bug 897194

(DevTools :: Inspector, defect)

Description

[[:Aleksej]]

hang: 2013-10-16-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64


Steps to reproduce (from bug 888627, but no need to be as fast):

1. Open any two tabs (it might help if the tab for step 2 has a page which makes Inspector start slower).
2. Open Inspector, e.g. by clicking a menu item ("Inspect element" in the context menu, or Tools → Developer Tools → Inspector)
3. Quickly switch to the other tab (I used Alt+number).

Nightly hangs and is occupying more and more RAM.  Eventually a slow-script warning may try to appear, but not become usable.

kill -ILL:
bp-273f13dd-8f4e-4217-8050-0c0e52131016	17/10/13	01:51
bp-9ae1b71f-0f2f-41b3-84ae-f29e32131016	17/10/13	01:46
bp-09352d61-ccf8-4a9c-97e6-3eac22131016	17/10/13	01:44
bp-076e7b5e-0e00-4992-a71e-b75972131016	17/10/13	00:47
bp-df0a9cbb-5ac9-43e7-a1a0-7fcba2131016	17/10/13	00:44
bp-324df469-fe73-41fd-a1e4-180a42131016	17/10/13	00:41
bp-02b35ebe-32e0-4915-aa8a-91b652131016	17/10/13	00:33
bp-e4783373-e5cc-41f6-9672-4de622131016	16/10/13	23:38 

5 of them are [@ libpthread-2.17.so@0xb974 ]



Regression range search ("reverse" and "WFM" are from the POV of bug 888627).

bug 888627: 2013-01-07-03-09-32-mozilla-central-firefox-20.0a1.en-US.linux-x86_64

WFM (inspector appears before I can switch tabs, tab switches back to the right tab when inspector appears there; still, maybe I was too slow): 2013-05-12-03-09-08-mozilla-central-firefox-23.0a1.en-US.linux-x86_64

reverse (clicked inspect on the AMO tab, switched to MDN tab, it auto-switched to the AMO tab and showed there inspector for MDN): 2013-07-28-03-02-04-mozilla-central-firefox-25.0a1.en-US.linux-x86_64
reverse: 2013-09-09-03-02-04-mozilla-central-firefox-26.0a1.en-US.linux-x86_64
reverse (doesn't switch back): 2013-09-26-11-20-34-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
seen both bug 888627 and reverse: 2013-10-06-03-02-01-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
reverse: 2013-10-08-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64
reverse: 2013-10-10-03-02-02-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 aa986b6ce882

hang: 2013-10-11-03-02-01-mozilla-central-firefox-27.0a1.en-US.linux-x86_64 6b101d4c6d24

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aa986b6ce882&tochange=6b101d4c6d24

Comment 1

[Alice0775 White]

Regression window(fx-team)
Good:
http://hg.mozilla.org/integration/fx-team/rev/4888e018458d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131010074250
Bad:
http://hg.mozilla.org/integration/fx-team/rev/dc3d80fd48b0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131010080051
Pushlog:
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=4888e018458d&tochange=dc3d80fd48b0

In local build
Last Good: 96e6d3d323cd
First Bad: 4caa766a7833
Regressed by:
	4caa766a7833	Brian Grinstead — Bug 923166 - Use nsIDOMWindowUtils.containerElement inside of LayoutHelpers.getFrameElement. r=paul

Comment 2

[[:Aleksej]]

This hang blocks reproduction of bug 888627.

Comment 3

[Brian Grinstead [:bgrins]]

Easiest / most consistent way I've found to reproduce:

1) Open two tabs
2) Focus first tab, put mouse directly over second tab
3) Press cmd/alt/c to open inspector and immediately click on the second tab.

When adding the following code to getFrameElement():

    console.trace();
    console.log("About to get frame element", win.location.toString(), this.isTopLevelWindow(win));

The following output gets spit out in an infinite loop (where https://www.mozilla.org/en-US/ is the SECOND tab that is being switched to):

    _s/LayoutHelpers.jsm 379 LH_getFrameElement
    _s/LayoutHelpers.jsm 83 LH_getDirectyRect
    _ctor/highlighter.js 259 LocalHighlighter_invalidateSize
    _ctor/highlighter.js 279 LocalHighlighter.prototype.show
    _ctor/highlighter.js 224 LocalHighlighter_highlight
    _ed/event-emitter.js 110 EventEmitter_emit
    _pector/selection.js 166 Selection.prototype.setNodeFront
    _/inspector-panel.js 136 InspectorPanel.prototype._deferredOpen/<
    _ed/event-emitter.js 63 EventEmitter_once/handler<
    _ed/event-emitter.js 110 EventEmitter_emit
    _/inspector-panel.js 619 InspectorPanel__onMarkupFrameLoad
    _/inspector-panel.js 601 InspectorPanel_initMarkupPanel_onload
                         0 

    console.log: About to get frame element https://www.mozilla.org/en-US/ false

In this case, the fact that it is calling with the wrong page (the second tab) is causing the problems, both here and in Bug 888627.  I'm looking into this further.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: 927630-debug.patch

Joe/Dave: I could use someone familiar with the remote inspector to take a look at this.

I've done a lot of tracking to figure out what is going on here.  And as far as I can tell, InspectorActor is being initialized with the wrong tab's window (the second one, that is being switched to).

I have attached a patch that prevents the crash and adds some additional logging.  I am not sure exactly where the tabActor is being passed into the InspectorActor, and if there is a way that we can get a handle to the tab that was active when the devtools originally were opened, not the new tab that was quickly switched to.  One thing to note is that inside of the toolbox, `this.target.url` remains correct even after the inspector is initialized with the wrong window.  I imagine if we fixed this, it would also fix Bug 888627 (as it appears to be the same underlying issue).

Following the STR in Comment 3 with this patch applied on two open tabs, starting at about:sessionrestore and going to about:newtab, here is what I see:

    console.log: Toolbox open beginning about:sessionrestore
    console.log: Toolbox open: DOMReady about:sessionrestore
    console.log: Before initializing inspector
    console.log: InspectorActor initialized about:newtab   <--- this should be about:sessionrestore just like the others
    console.log: After initializing inspector
    console.log: Toolbox open: Tool has been selected about:sessionrestore

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: 927630-debug.patch

Refreshed patch with additional logging

Comment 6

[Dave Camp (:dcamp)]

I have seen this needinfo request and nothing comes to mind immediately.  I'll try to take a look soon to see if I can figure something out.

Comment 7

[Brian Grinstead [:bgrins]]

I wonder if the safest thing to do would be to back out Bug 923166 for FF 27, then reland it later with a fix for this hanging problem.

Comment 8

[Dave Camp (:dcamp)]

LayoutHelpers will loop infinitely if it's asked about a node that isn't in its document hierarchy.  There might be something we can do to make that failure case throw rather than loop, but the real problem is that bug 896194 is causing that to actually happen.

Bug 897194 (also tracked) should be enough to stop tracking this bug, but we might leave this open to put in the protection in the failure case.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: istoplevel.patch

A tiny patch to double check that we have the correct window in LayoutHelpers.isTopLevelWindow.  As dcamp mentioned in Comment 8, this may not be necessary for this particular issue once Bug 897194 is resolved, but I think we should still update this, since it is more correct this way and could prevent a crash if this case comes up again.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=ebb1f4f4c79c

Comment 10

[Dave Camp (:dcamp)]

Comment on attachment 821320 [details] [diff] [review]
istoplevel.patch

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

::: toolkit/devtools/LayoutHelpers.jsm
@@ +340,5 @@
>      let docShell = win.QueryInterface(Ci.nsIInterfaceRequestor)
>                     .getInterface(Ci.nsIWebNavigation)
>                     .QueryInterface(Ci.nsIDocShell);
>  
> +    return win.parent === win || docShell === this._topDocShell;

I think there are some cases (docShell.isBrowserOrApp, see docShell.isBrowserOrApp) where the document will think it's a parent (win.parent === win) but where we want to continue going up.

It should be enough to just throw an error in getParentWindow() if we get to that spot and win === win.parent

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 821320 [details] [diff] [review]
istoplevel.patch

We can't rely on win.parent or win.top. This would return true in case of mozapps.

Comment 12

[Brian Grinstead [:bgrins]]

This hang is resolved by Bug 897194, which is fixed in Firefox 26.