Closed Bug 972341 Opened 6 years ago Closed 6 years ago

document.hidden should be true for xul:browsers in hidden windows

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(6 files, 15 obsolete files)

13.29 KB, patch
smaug
: review-
Details | Diff | Splinter Review
1.74 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
1.00 KB, patch
markh
: review+
Details | Diff | Splinter Review
1.11 KB, patch
Details | Diff | Splinter Review
14.14 KB, patch
Details | Diff | Splinter Review
2.11 KB, patch
Details | Diff | Splinter Review
document.hidden returns false for <xul:browser> elements loaded in hidden windows. It would be great if that returned false as expected and would then emit visibility change events after the docShell has been swapped into a visible tab.
Blocks: 910036
While this works fine I expect that we can't do this as it would break in e10s, no? If we have a <xul:browser remote="true">, would we need to send messages?

I would also like to write tests once I have some more feedback on the approach.
Attachment #8375476 - Flags: feedback?(bzbarsky)
I may be jumping the gun here but with document.hidden working correctly for browsers in the hidden window we can simplify some changes from 910036.
Attachment #8375487 - Flags: review?(adw)
Comment on attachment 8375487 [details] [diff] [review]
0002-Bug-972341-Simplify-changes-from-bug-910036-to-use-d.patch

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

Nice!

::: browser/base/content/newtab/page.js
@@ +18,5 @@
>  
>      // Listen for 'unload' to unregister this page.
>      addEventListener("unload", this, false);
>  
> +    // Listen for visibility changes to start capturing missing thumbnails.

We listen for only one visibility change, so our intent here would be made clearer by making "changes" singular.  "Wait for the document to become visible..." would make it clearer still.

@@ +143,5 @@
>            aEvent.stopPropagation();
>          }
>          break;
> +      case "visibilitychange":
> +        if (!document.hidden) {

Since the listener is only added when document.hidden and it's removed here, will this conditional ever be false?
Attachment #8375487 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #3)
> We listen for only one visibility change, so our intent here would be made
> clearer by making "changes" singular.  "Wait for the document to become
> visible..." would make it clearer still.

Agreed, that's better.

> Since the listener is only added when document.hidden and it's removed here,
> will this conditional ever be false?

No, good catch. Thanks!
Comment on attachment 8375476 [details] [diff] [review]
0001-Bug-972341-document.hidden-should-be-true-for-xul-br.patch

Do we want to only affect the document visibility state?  Or should the docshell itself be marked as inactive?

That is, do we want to only affect the visibility API or also freeze the refresh driver, mark the presshell as inactive, etc, etc?  Oh, and also correctly propagate to out-of-process descendants.  And it would automatically pick up the right state when swapping in/out of kids, since moving docshells around in the tree grab's the parent's active state...

Seems to me like we should simply set the root docshell of the hidden window as inactive; I'm a bit surprised we're not doing that already.
In fact, somewhere in nsAppShellService::CreateHiddenWindowHelper we could take the bit that grabs the docshell, move it out of the if/else bit and SetIsActive(false) there.

The only worry is whether this will cause issues on Mac with the menu, I guess.
Attachment #8375476 - Flags: feedback?(bzbarsky)
Do we slow down non-refresh-driver timers?  That could screw up the SocialAPI's frame worker.
We slow down setTimeout/setInterval in inactive windows, yes.
(In reply to Boris Zbarsky [:bz] from comment #5)
> That is, do we want to only affect the visibility API or also freeze the
> refresh driver, mark the presshell as inactive, etc, etc?  Oh, and also
> correctly propagate to out-of-process descendants.  And it would
> automatically pick up the right state when swapping in/out of kids, since
> moving docshells around in the tree grab's the parent's active state...

I think we only want to affect the visibility API to let pages know they are not visible. Marking the docShell as inactive would probably screw up the SocialAPI (as Kyle said), the about:newtab preloader that preloads pages in the hidden window and the OOP thumbnail creation that as well maintains a browser in the hidden window.

Now that I think about it, could this change possible screw up thumbnails because pages behave differently with hidden=true? Possibly, although I wouldn't really worry too much about that.
> I think we only want to affect the visibility API to let pages know they are not visible. 

That seems really really hacky to me, honestly...

If you do want to do it that way, you need a reliable way to know when you're entering/leaving the "hidden window", however that's defined.

Preloaded pages _should_ be marked as hidden in the docshell sense, imo, as should thumbnails.  You don't want those spamming the refresh driver and event loop, I would think, just like you don't want background tabs doing it.
No longer blocks: 971171
This patch makes CreateHiddenWindowHelper() grab the hidden window's docShell and set it inactive. It also introduces DocShell::SetIsActiveInternal() to allow setting the active state of chrome docshells because I thought we probably still wouldn't want to allow access to that from JavaScript.
Attachment #8375476 - Attachment is obsolete: true
Attachment #8376831 - Flags: review?(bzbarsky)
This second part takes care of passing the parent docShell's active state to remote browsers.
Attachment #8376832 - Flags: review?(bzbarsky)
Tests are missing currently but I would get started on those after having some more feedback. From some quick local test runs it looks like the social code and thumbnailing still work. I can't see anything wrong with the Mac menu either so far. How can we check that those won't break? I will push to try either way.
Another note: I did some smoke tests to check whether the preloader and OOP thumbnailing have inactive docShells and that works fine with the attached patches.
Try looks good, except for Jetpack tests. I wonder if we should allow activating chrome docShells but not disabling them. So the social frame worker and Jetpack could explicitly opt into active docShells in the hidden window. Or maybe they could do without that with a little rewrite.

https://tbpl.mozilla.org/?tree=Try&rev=4583966ff8cc
Comment on attachment 8376831 [details] [diff] [review]
0001-Bug-972341-Mark-a-hidden-window-s-docShell-as-inacti.patch

Please add the missing space before "void" and mark the method as notxpcom as well, and r=me.
Attachment #8376831 - Flags: review?(bzbarsky) → review+
Comment on attachment 8376832 [details] [diff] [review]
0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch

Why is this using the root item, not parentAsItem?

Also, now that I look again I don't see anything that would propagate the active state to the kid if it changes dynamically... am I just missing it?
Attachment #8376832 - Flags: review?(bzbarsky) → review-
What would the best way be to keep remote browsers in the loop about changes to their parent docShells' active state? My first thought was to add mRemoteBrowser as a nsIDocShellTreeItem to the tree and handle those in SetIsActiveInternal() but unfortunately SetIsActiveInternal() uses the nsDocLoader child list.

In the future we may want to propagate changes to the allow* flags as well like we do for non-remote docShells currently. So either we find something that works for all of those or defer this to a later moment.
Flags: needinfo?(bzbarsky)
> What would the best way be to keep remote browsers in the loop about changes to their
> parent docShells' active state?

Probably having the frameloader explicitly tell the parent docshell about remote browsers or something; some sort of add/remove operation to parallel what frameloader does with a docshell child.  Then we can propagate whatever state we want in the add?
Flags: needinfo?(bzbarsky)
Ok, so you say we should maintaining a separate list of remote browsers in the docShell. That should work, thanks.
Will work on tests in a separate patch.
Attachment #8378280 - Attachment is obsolete: true
Attachment #8378581 - Flags: review?(bzbarsky)
It's probably better to exclude browser and app frames from this visibility propagation. It unbreaks mochitest-2 and doesn't touch b2g that way.
Attachment #8378581 - Attachment is obsolete: true
Attachment #8378581 - Flags: review?(bzbarsky)
Attachment #8379022 - Flags: review?(bzbarsky)
(In reply to Tim Taubert [:ttaubert] from comment #16)
> Try looks good, except for Jetpack tests. I wonder if we should allow
> activating chrome docShells but not disabling them. So the social frame
> worker and Jetpack could explicitly opt into active docShells in the hidden
> window. Or maybe they could do without that with a little rewrite.

Looks like the latest patch doesn't do this? I'm not clear on what effect the checks from comment 25 have (i.e. whether OwnerIsBrowserOrAppFrame() is true for the jetpack/social frames).

Makes me nervous to have jetpack and social docshells be marked "inactive", since those are used for functionality that if often exposed externally in some other way (e.g. it could be bad if a setInterval in the frameworker script is throttled). I don't think we have good test coverage for this, unfortunately...
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #26)
> (In reply to Tim Taubert [:ttaubert] from comment #16)
> > Try looks good, except for Jetpack tests. I wonder if we should allow
> > activating chrome docShells but not disabling them. So the social frame
> > worker and Jetpack could explicitly opt into active docShells in the hidden
> > window. Or maybe they could do without that with a little rewrite.
> 
> Looks like the latest patch doesn't do this? I'm not clear on what effect
> the checks from comment 25 have (i.e. whether OwnerIsBrowserOrAppFrame() is
> true for the jetpack/social frames).

Yeah, the latest patch still breaks JP tests. I should have mentioned that I don't plan to land it in its current version without addressing test failures, of course.

> Makes me nervous to have jetpack and social docshells be marked "inactive",
> since those are used for functionality that if often exposed externally in
> some other way (e.g. it could be bad if a setInterval in the frameworker
> script is throttled). I don't think we have good test coverage for this,
> unfortunately...

Yeah, I kind of figured that our test suite might not expose problems with the patch. I thought about having an attribute that enables (or disables) visibility propagation that we could use to keep jetpack and social behavior as is. I'd rather have one that disables it so that new features run into and handle it properly when being built.
(In reply to :Gavin Sharp (email gavin@gavinsharp.com) from comment #26)
> Makes me nervous to have jetpack and social docshells be marked "inactive",
> since those are used for functionality that if often exposed externally in
> some other way (e.g. it could be bad if a setInterval in the frameworker
> script is throttled). I don't think we have good test coverage for this,
> unfortunately...

This patch introduces docShell.setForceActive(bool) which can be used to force an active docShell no matter what its parent state is. The patch changes the social frame worker to use that, adapting Jetpack is easy as well.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8380362 - Flags: review?(bzbarsky)
This patch fixes a11y m-oth tests using docShell.setForceActive(true) for the hiddenWindow docShells. We don't create accessibles for inactive docShells so we need to provide what the test assumes.

https://tbpl.mozilla.org/?tree=Try&rev=afc40fad48e0
I have a patch that fixes the Jetpack failures by using docShell.setForceActive(true) when creating a new frame in the hidden window. I'll need to figure out how to submit patches to Jetpack and how to run those on try.
Re-based and changed a few things since bug 978540 landed.
Attachment #8379022 - Attachment is obsolete: true
Attachment #8379022 - Flags: review?(bzbarsky)
Attachment #8384611 - Flags: review?(bzbarsky)
Simple rebase.
Attachment #8380362 - Attachment is obsolete: true
Attachment #8380362 - Flags: review?(bzbarsky)
Attachment #8384612 - Flags: review?(bzbarsky)
Comment on attachment 8384611 [details] [diff] [review]
0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch, v5

I just noticed that Boris says he's overloaded with reviews.
Attachment #8384611 - Flags: review?(bugs)
Attachment #8384612 - Flags: review?(bugs)
This review is one of the main reasons I say that.  I do plan to get to it today or worst-case tomorrow.
Comment on attachment 8384611 [details] [diff] [review]
0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch, v5

This doesn't deal with frameloader swapping in any way.

Also, I don't understand why !OwnerIsBrowserOrAppFrame()
checks?
Attachment #8384611 - Flags: review?(bugs) → review-
Attachment #8384612 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #35)
> This doesn't deal with frameloader swapping in any way.

Does it have to? SwapWithOtherLoader() just bails out if GetExistingDocShell() returns null which AFAICT means we don't support docShell swapping for remote browsers.

https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1074

> Also, I don't understand why !OwnerIsBrowserOrAppFrame() checks?

The main purpose of this patch was to correct behavior for Firefox and its tabbrowsers. B2G has its own mechanism of propagating visibility to mozbrowser iframes when switching applications and this would probably break with correct propagation. I think this would be best addressed in a follow-up. I don't know enough about standalone app frames to say whether those would break as well, so I excluded them here.
(In reply to Tim Taubert [:ttaubert] from comment #36)
> (In reply to Olli Pettay [:smaug] from comment #35)
> > This doesn't deal with frameloader swapping in any way.
> 
> Does it have to? SwapWithOtherLoader() just bails out if
> GetExistingDocShell() returns null which AFAICT means we don't support
> docShell swapping for remote browsers.
> 
> https://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsFrameLoader.cpp#1074

Oh, crap. That is broken. Let's fix that in a followup. Filed Bug 979239.


> > Also, I don't understand why !OwnerIsBrowserOrAppFrame() checks?
> 
> The main purpose of this patch was to correct behavior for Firefox and its
> tabbrowsers. B2G has its own mechanism of propagating visibility to
> mozbrowser iframes when switching applications and this would probably break
> with correct propagation. 
Why would it? I don't assume b2g to use isActive stuff.
Didn't find any use of it in b2g/

> I think this would be best addressed in a
> follow-up. I don't know enough about standalone app frames to say whether
> those would break as well, so I excluded them here.
I'm worried about adding inconsistencies to the platform, which may never get fixed.
I've tried to get rid of use of OwnerIsBrowserOrAppFrame() checks.
(In reply to Olli Pettay [:smaug] from comment #37)
> Why would it? I don't assume b2g to use isActive stuff.
> Didn't find any use of it in b2g/

https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#881

> I'm worried about adding inconsistencies to the platform, which may never
> get fixed.
> I've tried to get rid of use of OwnerIsBrowserOrAppFrame() checks.

Fair enough, I don't like the special casing of that either.
Comment on attachment 8384611 [details] [diff] [review]
0002-Bug-972341-Propagate-docShell.isActive-to-remote-bro.patch, v5

I guess Olli is on top of this at this point.  I'm sorry for the lag...  :(
Attachment #8384611 - Flags: review?(bzbarsky)
Attachment #8384612 - Flags: review?(bzbarsky)
This patch is quite similar to the previous ones but it does two more things:

1) I removed the OwnerIsBrowserOrAppFrame() checks from the nsFrameLoader. TabChild::RecvSetIsDocShellActive() however still checks it and just does nothing for browser or app frames. B2G has its very own way of dealing with docShell active states and it would be great to fix that in a follow-up rather to have it block this bug.

2) nsDocShell::SetIsActive() does now allow changing a chrome docShell's active state to true. That is a great way to allow the social worker and the add-on SDK to force active docShells. I assume the condition was built in to prevent chrome docShells from becoming inactive.
Attachment #8376826 - Attachment is obsolete: true
Attachment #8378220 - Attachment is obsolete: true
Attachment #8380609 - Attachment is obsolete: true
Attachment #8384611 - Attachment is obsolete: true
Attachment #8384612 - Attachment is obsolete: true
Attachment #8424512 - Flags: review?(bugs)
Forcing social frame worker docShells to be active.
Attachment #8424514 - Flags: review?(mhammond)
Forcing add-on SDK frames to be active.
Attachment #8424516 - Flags: review?(dtownsend+bugmail)
Updated patch to simplify some about:newtab code.
Attachment #8424517 - Flags: review?(adw)
Attachment #8424514 - Flags: review?(mhammond) → review+
Attachment #8424516 - Attachment is obsolete: true
Attachment #8424516 - Flags: review?(dtownsend+bugmail)
Attachment #8424512 - Attachment is obsolete: true
Attachment #8424512 - Flags: review?(bugs)
Attachment #8424517 - Attachment is obsolete: true
Attachment #8424517 - Flags: review?(adw)
Attachment #8424516 - Attachment is obsolete: false
Attachment #8424516 - Flags: review?(dtownsend+bugmail)
Attachment #8424512 - Attachment is obsolete: false
Attachment #8424512 - Flags: review?(bugs)
This should fix a11y m-oth try failures.
Attachment #8424747 - Flags: review?(trev.saunders)
Forgot to remove an occurrence of |allowBackgroundCaptures| which caused try timeouts.
Attachment #8424764 - Flags: review?(adw)
Comment on attachment 8424747 [details] [diff] [review]
0006-Bug-972341-Wait-until-hidden-frames-are-ready-before.patch

Argh, those fragile tests drive me nuts. Still failing on try :(
Attachment #8424747 - Flags: review?(trev.saunders)
Comment on attachment 8424512 [details] [diff] [review]
0001-Bug-972341-Propagate-docShell-active-states-to-remot.patch

This really need to be split to two pieces.
Or setIsActiveInternal should be removed.
It is really odd to see such method in a public interface.

Why would we ever want to set chrome docshell inactive?
It would be odd to see chrome setTimeout to not support < 1s values.
Attachment #8424512 - Flags: review?(bugs) → review-
This looks quite promising. We don't need to care about cleaning up as a11y tests have their own test run and the browser will restart before running all other mochitests.
Attachment #8424747 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #48)
> This really need to be split to two pieces.

Fair enough, I can totally split this into smaller parts.

> Why would we ever want to set chrome docshell inactive?
> It would be odd to see chrome setTimeout to not support < 1s values.

When I started to work on this issue I tried to make document.hidden return true just based on the fact that it belonged to the hidden window. In comment #5 Boris pointed out that the right solution would be to mark the docShell as actually hidden. What's the suggested way forward now?
Oh, I'm not actually sure what bz means with root docshell there. Root docshell in the child process
(that would be a content docshell - at least usually), or root docshell in the parent process.
That would be a chrome docshell.
Making a chrome docshell to not have normal timeout handling etc. would be rather regression prone.
Flags: needinfo?(bzbarsky)
Anyhow, I'd make the root content docshell active/inactive (doesn't matter whether the docshell is in parent or child process)
> Making a chrome docshell to not have normal timeout handling etc. would be rather
> regression prone.

Sure.  I was specifically talking about the hidden window.

> I'd make the root content docshell active/inactive

That becomes the responsibility of everyone injecting junk into the hidden window then, right?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #53)
> That becomes the responsibility of everyone injecting junk into the hidden
> window then, right?

I don't actually remember whether we use chrome or content docshell as the toplevel shell for hidden window, but anyhow, making something inactive there should be, IMO, responsibility of the user of the
hidden window.
Then why do we need any core changes at all?  Users of the hidden window can already do that, right?
(In reply to Boris Zbarsky [:bz] from comment #55)
> Then why do we need any core changes at all?  Users of the hidden window can
> already do that, right?

Yeah, that way around it would have been a lot easier :| I'll move part 0005 to bug 973532 and wontfix this one. Thanks for clearing this up.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Attachment #8424516 - Flags: review?(dtownsend+bugmail)
Attachment #8424764 - Flags: review?(adw)
(In reply to Olli Pettay [:smaug] from comment #54)
> (In reply to Boris Zbarsky [:bz] from comment #53)
> > That becomes the responsibility of everyone injecting junk into the hidden
> > window then, right?
> 
> I don't actually remember whether we use chrome or content docshell as the
> toplevel shell for hidden window, but anyhow, making something inactive
> there should be, IMO, responsibility of the user of the
> hidden window.

Thinking a little about this I agree even more that this is the right solution. There are so many users of the hidden window and making all of their docShells inactive could have unintended consequences. Having everyone be responsible for their own docShells makes a lot of sense.
Olli, are we still interested in forwarding docShell activity state changes to remote browsers? Should I split that off into a new bug? The attached patches should implement that already.
Flags: needinfo?(bugs)
Having some easy way for parent to in-activate child-process docshells sounds useful.
Flags: needinfo?(bugs)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.