1464747 - Browser toolbox can't hightlight frame again.

Status: VERIFIED FIXED

(DevTools :: General, defect, P2)

Description

[Mantaroh Yoshinaga[:mantaroh]]

Browser toolbox will freeze if hover a frame popup.

STR:
 1. Open browser toolbox.
 2. Choose the any frame(like. chrome://extensin/content/dummy.xul)
 3. choose the frame again after swithing the frame.
 4. hover popup menu.

AR:
 Browser Toolbox process freezed.

I looked into this a little bit. As far as I can see, 'target.getActorDesciption' didn't resolve[1].

[1] https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/devtools/client/framework/target.js#195

Comment 1

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #0)
> Browser toolbox will freeze if hover a frame popup.
> 
> STR:
>  1. Open browser toolbox.
>  2. Choose the any frame(like. chrome://extensin/content/dummy.xul)
>  3. choose the frame again after swithing the frame.
>  4. hover popup menu.
> 
> AR:
>  Browser Toolbox process freezed.
> 
> I looked into this a little bit. As far as I can see,
> 'target.getActorDesciption' didn't resolve[1].
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 5a744713370ec47969595e369fd5125f123e6d24/devtools/client/framework/target.
> js#195

Ugh, this comment's investigation is wrong. The main reason is the following:

1) Browser toolbox received the frames like following frames, and created the map which key is frame.id.
     -------------------------------------------------------
     | frame.id | parentID | content                       |
     |    1     | undefined| browser.xul                   |
     |    6     |    1     | about:blank                   |
     |    10    |    10    | dummy.xul(WebExt)             |
     |    14    |    14    | background_page.html(WebExt)  |
     |    17    |    17    | dummy.xul(WebExt)             |
     |    21    |    21    | background_page.html(WebExt)  |
     -------------------------------------------------------

2) toolbox will search root frame by using frame.parentID until frame.parentID is undefined.[1]

3) As result of 2), browser toolbox go into infinite loop. For example, If current selected frame is 10, parentID is '10'. So current calculation will get the same frame permanently.

We need to prevent this infinite loop in browser toolbox.

Comment 2

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #1)
> 2) toolbox will search root frame by using frame.parentID until
> frame.parentID is undefined.[1]
> 

Sorry. Target code is [1] https://searchfox.org/mozilla-central/rev/38bcf897f1fa19c1eba441a611cf309482e0d6e5/devtools/client/framework/toolbox.js#2424-2437.

Comment 3

[Mantaroh Yoshinaga[:mantaroh]]

Attachment: Bug 1464747 - Doesn't set the parentID of frame when window.parent is same as window.

In order to prevent infinite loop, this patch will add the break condition of
searching parent frame.

Browser toolbox might have the frames which is 'parent id === current frame id'.
In this case, we need to stop loop of searching parent frame.

Review commit: https://reviewboard.mozilla.org/r/248360/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248360/

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8982407 [details]
Bug 1464747 - Doesn't set the parentID of frame when window.parent is same as window.

https://reviewboard.mozilla.org/r/248360/#review255110

Thanks for investigating this!

I think it would be easier for future maintenance if we instead fix this by correcting the `parentID` to be null for such frames server side.  It looks like they are determined by `_docShellToWindow`[1]:

```
    let parentID = undefined;
    if (window.parent && window != this._originalWindow) {
      parentID = ...
    }
```

If we modify this to check whether the parent window is different from the current window, I think it would set such cases to undefined:

```
    let parentID = undefined;
    if (window.parent && window.parent != window && window != this._originalWindow) {
      parentID = ...
    }
```

What do you think about this approach?  This way we'll have correct data, and we won't need to work around it in the front end.

[1]: https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/devtools/server/actors/tab.js#778

Comment 5

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982407 [details]
Bug 1464747 - Doesn't set the parentID of frame when window.parent is same as window.

https://reviewboard.mozilla.org/r/248360/#review255110

Thank you for the review.

I never thought modifying the server side. However, to modify the server side is reasonable for me rather than changing front end. I'll do this.

Comment 6

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8982407 [details]
Bug 1464747 - Doesn't set the parentID of frame when window.parent is same as window.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248360/diff/1-2/

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8982407 [details]
Bug 1464747 - Doesn't set the parentID of frame when window.parent is same as window.

https://reviewboard.mozilla.org/r/248360/#review255348

Thanks, this looks good to me! :)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7be4ff6e554f
Doesn't set the parentID of frame when window.parent is same as window. r=jryans

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/7be4ff6e554f

Comment 10

[Peter Magyari (Desktop QA)]

Verified as Fixed on Firefox 64.0.2 Build ID:20190108160530 on Windows 10 (64-bit).