1405342 - Can't make inactive devtools (chrome) iframes

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

In bug 1399242, we would like to make devtools panel which goes to background be hidden and inactive. To that we would like to use visibilitychange event and document.visiblityState attribute. But devtools still use chrome document loaded via <xul:iframe>.

Setting `docShell.isActive=false` throws NS_ERROR_ILLEGAL_VALUE,
very likely because of that:
http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/docshell/base/nsDocShell.cpp#6320-6323
  // We disallow setting active on chrome docshells.
  if (mItemType == nsIDocShellTreeItem::typeChrome) {
    return NS_ERROR_INVALID_ARG;
  }

I was wondering if this check was really justified and/or if there is anything we can do on devtools side to make devtools iframe be inactive?

We still use <xul:iframe> without any "type" attribute:
http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#1615-1621
    iframe = this.doc.createElement("iframe");
    iframe.className = "toolbox-panel-iframe";
    iframe.id = "toolbox-panel-iframe-" + id;
    iframe.setAttribute("flex", 1);
    iframe.setAttribute("forceOwnRefreshDriver", "");
    iframe.tooltip = "aHTMLTooltip";
    iframe.style.visibility = "hidden";

And we load chrome urls like this:
  chrome://devtools/content/inspector/inspector.xhtml
  chrome://devtools/content/webconsole/webconsole.xhtml

In the meantime, I made a hack to fake a visibilitychange event and overload document.visibilityState but I imagine we would benefit from proper platform support!

Comment 1

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

The comment about not allowing for chrome docshells appeared when setting active was first added, in bug 343515:

https://hg.mozilla.org/mozilla-central/rev/b7836c3a63db

My guess is there just wasn't an imagined need for it with chrome docs at the time (~7 years ago).  I would imagine the check could be removed with proper tests in place.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1405342 - Allow making chrome iframe be inactive.

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

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

Try is 99.9% green, but I think the only failing test highlights something wrong.

https://treeherder.mozilla.org/logviewer.html#?job_id=135953322&repo=try&lineNumber=3374
TEST-PASS | accessible/tests/mochitest/events/test_docload.html | The document accessible for '../events/docload_wnd.html' is not in cache! 
TEST-PASS | accessible/tests/mochitest/events/test_docload.html | The document accessible for iframe is in cache still! 
Buffered messages finished
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/events/test_docload.html | Scenario #0 of test with ID = 'open dialog '../events/docload_wnd.html'' failed. Dupe reorder event.

This test involves an iframe visibility:
http://searchfox.org/mozilla-central/source/accessible/tests/mochitest/events/test_docload.html#143
  this.invoke = function makeIFrameVisible_invoke() {
    this.DOMNode.style.visibility = "visible";
  }

I'll try to dig that particular failure but wanted your overall thinking on this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

https://reviewboard.mozilla.org/r/188014/#review193566

> My guess is there just wasn't an imagined need for it with chrome docs at the time (~7 years ago)

If you actually read the discussion in bug 343515 you will see some concerns about this API being a potential footgun on chrome docshells, which is why we disallowed it at the time.  See bug 343515 comment 23, bug 343515 comment 25.

You're correct that there were no pressing use cases at the time, so the combination of that and the fact that it was so easy to misuse is what led to us disallowing it.

We could probably remove the restriction and hope that people don't do that.  We would need to double-check (and add tests) that everything works correctly at the type boundary, though.

Another option would be to require that chrome docshells opt in to this API somehow (e.g. attribute on the element) and enforce no content docshells in the subtree rooted by such an opted-in docshell.  That seems a bit over-complicated, though, and possibly needlessly restrictive...

I'm probably OK with removing the check if we add good tests for the type-boundary case.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

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

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

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

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #4)
> We could probably remove the restriction and hope that people don't do that.
> We would need to double-check (and add tests) that everything works
> correctly at the type boundary, though.
...
> I'm probably OK with removing the check if we add good tests for the
> type-boundary case.

I tweaked the test to cover child frames being chrome or content and had to tweak SetIsActive to make it pass.
I hope I captured correctly what you had in mind?


https://treeherder.mozilla.org/#/jobs?repo=try&revision=9864e9af332d

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

https://reviewboard.mozilla.org/r/188014/#review194102

Can you explain what behavior you're trying to implement?  It's not entirely clear to me.

Specifically, with this patch as written, if I add a content child to a chrome docshell and then mark the chrome docshell inactive, the child will still be active.  But if I reverse the order of those two operations, the child will be inactive.  (Yes, we already had this problem for mozbrowser iframes, but luckily those are pretty much unused.)  Is that the intent?

Comment 9

[Alexandre Poirot [:ochameau]]

Per bug 343515 comment 23:
> 1)  You probably don't want to cross type boundaries when setting active on the
>     kids.

And bug 343515 comment 25:
> Yes.  So if someone calls SetActive(PR_TRUE) on the root chrome docshell for a Firefox window, it will happily call 
> SetActive(PR_TRUE) on every single tab in that window.  That seems wrong to me... but maybe it's ok.

My intent was:
 - chrome docshell will update only its chrome children.
 - content docshell behavior should not change.

But it looks like I missed that you actually can have a chrome docshell be a child of a content one.
Is that something that can exists?

In such case, I imagine the check should be:
   if (!docshell->GetIsMozBrowser() &&
       ((mItemType == nsIDocShellTreeItem::typeChrome &&
         docshell->ItemType() == nsIDocShellTreeItem::typeChrome) ||
        (mItemType != nsIDocShellTreeItem::typeChrome &&
         docshell->ItemType() != nsIDocShellTreeItem::typeChrome) {
      docshell->SetIsActive(aIsActive);

May be:
   if (!docshell->GetIsMozBrowser() &&
       mItemType == docshell->ItemType()) {
would work? but I don't really follow the various docshell types and their real differences.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> That you actually can have a chrome docshell be a child of a content one

No, that in fact cannot happen.

Maybe comment 8 wasn't very clear?  With your current patch, if you have a chrome parent X with a content child Y, the following things are true:

1)  When Y is added as a child of X, nsDocShell::SetDocLoaderParent will get called on Y and do:

    if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
      // a prerendered docshell is not active yet
      SetIsActive(value && !mIsPrerendered);
    }

which will pick up whatever the active state of the parent is.

2)  When active state is toggled on X, it will not update the active state on Y.

This inconsistency doesn't really make sense to me.  It would make sense to either consistently propagate state from chrome parent to content child or consistently not propagate it.

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188014/diff/3-4/

Comment 12

[Alexandre Poirot [:ochameau]]

Here is a new patch, with related test that should address your previous comment.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> This inconsistency doesn't really make sense to me.  It would make sense to
> either consistently propagate state from chrome parent to content child or
> consistently not propagate it.

If feel like you are not 100% convinced we should make a special case against chrome->content active propagation.
TBH, I'm not confident enough about this codebase to make the call.
I just found that your comments from bug 343515 made sense to me, but feel free to tell me we should keep things simple here.

Otherwise, debugging this a11y test is quite challenging and still failed on the previous version of this patch...

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

https://reviewboard.mozilla.org/r/188014/#review194522

r=me with some nits.

::: docshell/base/nsDocShell.cpp:3480
(Diff revision 4)
>      if (parentAsDocShell->GetIsPrerendered()) {
>        SetIsPrerendered();
>      }
> +    // To match SetIsActive propagation behavior, do not inherit active state
> +    // between chrome parent and  content children
> +    if (parentAsDocShell->ItemType() != nsIDocShellTreeItem::typeChrome ||

Just compare parentAsDocShell->ItemType() to mItemType.  If not equal, do not propagate.

::: docshell/base/nsDocShell.cpp:3484
(Diff revision 4)
> +    // between chrome parent and  content children
> +    if (parentAsDocShell->ItemType() != nsIDocShellTreeItem::typeChrome ||
> +        mItemType == nsIDocShellTreeItem::typeChrome) {
> -    if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
> +      if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
> -      // a prerendered docshell is not active yet
> +        // a prerendered docshell is not active yet
> -      SetIsActive(value && !mIsPrerendered);
> +        SetIsActive(value && !mIsPrerendered);

We probably want to SetIsActive(!mIsPrerendered) in the "else" clause here or something....

::: docshell/base/nsDocShell.cpp:6393
(Diff revision 4)
>      if (!docshell) {
>        continue;
>      }
>  
> -    if (!docshell->GetIsMozBrowser()) {
> +    if (!docshell->GetIsMozBrowser() &&
> +        (mItemType != nsIDocShellTreeItem::typeChrome ||

Again just check for mItemType == docshell->ItemType().

::: docshell/test/navigation/browser_bug1405342.js:35
(Diff revision 4)
> +  is(content.contentWindow.top, content.contentWindow, "#content is really a content iframe");
> +  is(chrome.contentWindow.top, window, "#chrome is really a chrome iframe");
> +
> +  ok(chrome.frameLoader.docShell.isActive, "Chrome child iframe is active after load");
> +  ok(content.frameLoader.docShell.isActive, "Content child iframe is active after load");
> +  ok(chrome.frameLoader.docShell.isActive, "Chrome child iframe is active after load");

This is a copy of line 33.  Why is it needed?

::: docshell/test/navigation/browser_bug1405342.js:56
(Diff revision 4)
> +  is(chromeInherit.contentWindow.top, window, "chromeInherit refers to a chrome iframe");
> +  is(contentInherit.contentWindow.top, contentInherit.contentWindow, "contentInherit refers to a content iframe");
> +  ok(!chromeInherit.frameLoader.docShell.isActive, "Chrome iframe inherits active state and is still inactive");
> +  ok(contentInherit.frameLoader.docShell.isActive, "Content iframe doesn't inherit active state and is active");
> +
> +  info("Waiting for visibiliychange event");

"visibilitychange"

Comment 14

[Alexandre Poirot [:ochameau]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> ::: docshell/base/nsDocShell.cpp:3484
> (Diff revision 4)
> > +    // between chrome parent and  content children
> > +    if (parentAsDocShell->ItemType() != nsIDocShellTreeItem::typeChrome ||
> > +        mItemType == nsIDocShellTreeItem::typeChrome) {
> > -    if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
> > +      if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
> > -      // a prerendered docshell is not active yet
> > +        // a prerendered docshell is not active yet
> > -      SetIsActive(value && !mIsPrerendered);
> > +        SetIsActive(value && !mIsPrerendered);
> 
> We probably want to SetIsActive(!mIsPrerendered) in the "else" clause here
> or something....

I went for:
  if (parentAsDocShell->ItemType() == mItemType) {
    if (NS_SUCCEEDED(parentAsDocShell->GetIsActive(&value))) {
      // a prerendered docshell is not active yet
      SetIsActive(value && !mIsPrerendered);
    }
  } else {
    // a prerendered docshell is not active yet
    SetIsActive(!mIsPrerendered);
  }

But not sure I'm quite following this !mIsPrerendered check.
It looks like we only want to prevent doing SetIsActive(true) if this is a pre-rendered.
We still apply this rule, there is no need for this new else branch?

> ::: docshell/test/navigation/browser_bug1405342.js:35
> (Diff revision 4)
> > +  is(content.contentWindow.top, content.contentWindow, "#content is really a content iframe");
> > +  is(chrome.contentWindow.top, window, "#chrome is really a chrome iframe");
> > +
> > +  ok(chrome.frameLoader.docShell.isActive, "Chrome child iframe is active after load");
> > +  ok(content.frameLoader.docShell.isActive, "Content child iframe is active after load");
> > +  ok(chrome.frameLoader.docShell.isActive, "Chrome child iframe is active after load");
> 
> This is a copy of line 33.  Why is it needed?

Copy paste mistake.


Now, I'm blocked by a11y bug 638313.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> It looks like we only want to prevent doing SetIsActive(true) if this is a pre-rendered.

I'd think we actively want to set it to false, since it defaults to true, no?  Right now we unconditionally call SetIsActive(false) if mIsPrerendered, and we should keep that behavior.

Comment 16

[Alexandre Poirot [:ochameau]]

Comment on attachment 8916986 [details]
Bug 1405342 - Allow making chrome iframe be inactive.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188014/diff/4-5/

Comment 17

[Alexandre Poirot [:ochameau]]

It looks like with bug 638313 and bug 1415115, a11y tests is now intermittent on m-c.
And this patch doesn't make it much more, nor less:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe2a12552c15f17f1696bc24eeff677acec759fc&selectedJob=144550092&filter-searchStr=a11y