Tim Taubert [:ttaubert] (inactive) Assignee
	Description • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 1 • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 2 • 11 years ago

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.

	Drew Willcoxon :adw
	Comment 3 • 11 years ago

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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 4 • 11 years ago

(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!

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 11 years ago

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.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 7 • 11 years ago

Do we slow down non-refresh-driver timers?  That could screw up the SocialAPI's frame worker.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 11 years ago

We slow down setTimeout/setInterval in inactive windows, yes.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 9 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

> 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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 12 • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 13 • 11 years ago

This second part takes care of passing the parent docShell's active state to remote browsers.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 14 • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 15 • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 16 • 11 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 11 years ago

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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 21 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 22 • 11 years ago

> 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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 23 • 11 years ago

Ok, so you say we should maintaining a separate list of remote browsers in the docShell. That should work, thanks.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 24 • 11 years ago

Will work on tests in a separate patch.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 25 • 11 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 26 • 11 years ago

(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...

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 27 • 11 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 28 • 11 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 29 • 11 years ago

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

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 30 • 11 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 31 • 11 years ago

Re-based and changed a few things since bug 978540 landed.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 32 • 11 years ago

Simple rebase.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 33 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 34 • 11 years ago

This review is one of the main reasons I say that.  I do plan to get to it today or worst-case tomorrow.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 35 • 11 years ago

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?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 36 • 11 years ago

(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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 37 • 11 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 38 • 11 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 39 • 11 years ago

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...  :(

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 40 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 41 • 10 years ago

Carrying over r=bz.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 42 • 10 years ago

Forcing social frame worker docShells to be active.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 43 • 10 years ago

Forcing add-on SDK frames to be active.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 44 • 10 years ago

Updated patch to simplify some about:newtab code.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 45 • 10 years ago

This should fix a11y m-oth try failures.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 46 • 10 years ago

Forgot to remove an occurrence of |allowBackgroundCaptures| which caused try timeouts.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 47 • 10 years ago

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 :(

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 48 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 49 • 10 years ago

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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 50 • 10 years ago

(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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 51 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 52 • 10 years ago

Anyhow, I'd make the root content docshell active/inactive (doesn't matter whether the docshell is in parent or child process)

	Boris Zbarsky [:bzbarsky]
	Comment 53 • 10 years ago

> 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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 54 • 10 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 55 • 10 years ago

Then why do we need any core changes at all?  Users of the hidden window can already do that, right?

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 56 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 57 • 10 years ago

(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.

	Tim Taubert [:ttaubert] (inactive) Assignee
	Comment 58 • 10 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 59 • 10 years ago

Having some easy way for parent to in-activate child-process docshells sounds useful.