Closed Bug 518805 Opened 10 years ago Closed 4 years ago

meta refresh should not reload page when tab/window is not active (not visible) or in idle state

Categories

(Core :: Document Navigation, defect)

1.9.2 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 + wontfix
firefox47 --- fixed
relnote-firefox --- 47+
fennec 46+ ---

People

(Reporter: romaxa, Assigned: snorp, NeedInfo)

References

(Blocks 2 open bugs, )

Details

Attachments

(1 file, 2 obsolete files)

pages with meta refresh tag will drain battery on mobile device.
I think we should take care about this problem.
I'm not sure we want to change the default behavior here; I can see providing an option for certain embeddings to have this behavior.
I agree... for default browser it is not very good to disable it by default (Some page can be reloaded and trow some notification (sound, window manager) about new state)..

But for mobile case it may cost you by dead phone...
I see 2 ways:
1) use patch from bug 343515, and have some preference which will enable/disable suspend on Docshell->SetActive call.
2) Provide separate API for this suspension...
The idea is to stop all refresh timers when we are not active and restore all states when we are active again
This patch should be apply on top of patch from bug 343515.
Attachment #407789 - Flags: review?(bzbarsky)
Why do you need the presshell changes?

What happened to the calling Freeze() idea?
(In reply to comment #4)
> Why do you need the presshell changes?
> 
> What happened to the calling Freeze() idea?

These PresShell changes are part of patch from bug 343515, didn't delete everything. Sorry.
Timers that I need to manipulate are in DocShell and they are private members there. ResumeRefreshURIs and SuspendRefreshURIs only stops and starts those timers. But after they are stopped new timers can be added in RefreshUri. So I need some flag that can tell if DocShell is active or not and to freeze all added timers.
This patch has better solution. All trash about PresShell is removed. Also should be applied upon patch from bug 343515.
Attachment #407789 - Attachment is obsolete: true
Attachment #409126 - Flags: review?(bzbarsky)
Attachment #407789 - Flags: review?(bzbarsky)
Comment on attachment 409126 [details] [diff] [review]
Handling meta refresh timers in background (better solution)

>+++ b/docshell/base/nsDocShell.cpp	Thu Oct 29 19:25:23 2009 +0200
> nsDocShell::SetIsActive(PRBool aIsActive)
>+    // Resume refresh URIs for our child shells as well.

This is fine, but should probably be done as part of bug 343515; it's needed for more than just refresh URIs.

>@@ -4986,21 +5008,21 @@ nsDocShell::RefreshURI(nsIURI * aURI, PR
>+    if (busyFlags & BUSY_FLAGS_BUSY || (!mIsActive && mDisableMetaRefreshInBackground)) {
>         // We are busy loading another page. Don't create the
>         // timer right now. Instead queue up the request and trigger the
>         // timer in EndPageLoad(). 

Perhaps the comment should now say:

  // We don't want to trigger the refresh right now.  Instead, queue up the
  // request and trigger the timer in EndPageLoad or when we become active.

?

>@@ -5799,22 +5821,23 @@ nsDocShell::EndPageLoad(nsIWebProgress *
>+    if (mIsActive)
>+        RefreshURIFromQueue();

That should be |mIsActive || !mDisableMetaRefreshInBackground|, no?

>@@ -10195,20 +10218,24 @@ nsDocShell::Observe(nsISupports *aSubjec
>+        rv = prefs->GetBoolPref("browser.meta_refresh_in_background.disabled", &tmpbool);
>+        if (NS_SUCCEEDED(rv))
>+            mDisableMetaRefreshInBackground = tmpbool;
>+

This change doesn't make sense.  That block where you added the code is the block where we get notified that the error pages pref changed.

If you think the meta_refresh_in_background pref should be live over a docshell lifetime, then we need to be observing it explicitly (so adding as an observer in Create(), and having a block in Observe() checking for it).

Personally, I'm ok with this particular pref requiring a new docshell to be picked up.

r=bzbarsky with the above issues fixed.  My apologies for the lag here....
Attachment #409126 - Flags: review?(bzbarsky) → review+
Egor please attach updated patch.
This is controlled by browser.meta_refresh_when_inactive.disabled, which
is false (allow refreshes) on desktop, and true (disable refreshes) on Fennec
Attachment #8725907 - Flags: review+
Attachment #409126 - Attachment is obsolete: true
Nominating for tracking-fennec: we should get this to release ASAP to mitigate background bandwidth consumption, which has been a thorn in our sides for years.
Assignee: nobody → starkov.egor
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
OS: Linux → All
Hardware: Other → All
https://hg.mozilla.org/mozilla-central/rev/adfae83c382e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
We can request uplift.
tracking-fennec: ? → 46+
Release Note Request (optional, but appreciated)
[Why is this notable]: Disables meta refresh on background web pages reducing Firefox's data usage
[Suggested wording]: stopped data usage when Firefox is in the background and a page uses meta refresh 
[Links (documentation, blog post, etc)]:
Assignee: starkov.egor → snorp
relnote-firefox: --- → ?
Ioana would someone from the Softvision team verify this? Best to use a powerful phone or tablet as being killed in the background would affect this test.

* Attach Firefox for Android to desktop devtools
* Switch to the network monitor
* Open http://www.lefigaro.fr/flash-actu/ in Firefox for Android
* ~5 to 7 MB of data should be transferred
* due to meta refresh tag the page will reload ~5 to ~7 MB every 15 min 

* If the screen is on with firefox as the active app, lefigaro as the active tab and 15 min have passed the page should reload
* If the screen is off with firefox as the active app, lefigaro as the active tab and 15 min have passed no data should be consumed 
* If the screen is on with firefox as the active app, lefigaro as the background tab and 15 min have passed no data should be consumed by lefigaro
* If the screen is on with firefox as the background app (switch to gmail or similar) , lefigaro as the active tab and 15 min have passed no data should be consumed by lefigaro
Flags: needinfo?(ioana.chiorean)
Reducing background bandwidth use sounds great, tracking this for 46
Can you request uplift to beta?
Flags: needinfo?(snorp)
Comment on attachment 8725907 [details] [diff] [review]
Don't do meta refreshes when backgrounded

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Unnecessary refreshes when Fennec is backgrounded, resulting in increased data usage and wasted battery.
[Describe test coverage new/current, TreeHerder]: Nightly, aurora
[Risks and why]: Low
[String/UUID change made/needed]: None
Flags: needinfo?(snorp)
Attachment #8725907 - Flags: approval-mozilla-beta?
Comment on attachment 8725907 [details] [diff] [review]
Don't do meta refreshes when backgrounded

Should improve battery life by reducing repaints. This should make it into the beta 4 build.
Attachment #8725907 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I don't think a risk assessment of "low" is ever reasonable for docshell.

Looking at the patch carefully again, I realized that we missed a case: what happens when the docshell goes active before EndPageLoad?
Flags: needinfo?(snorp)
Also, seems to me that what you may want to do is avoid doing the refresh, not avoid starting the timer, right?  Not entirely clear.

Oh, and while I was reading the diff again, "whenver" is not a word.
This may be a case where I could have asked for a 2nd review or just said no to uplifting this far.  
We can still back this out and do more verification, or just not aim for 46 as we have plenty of risk already.
Wes can you back this out on mozilla-beta before I build beta 4? I have cold feet.  

We could give ourselves a bit more time to figure out ways to do manual testing for this on both fennec and desktop, and for snorp to address the issues bz brought up.   I don't expect aurora android users to report issues on this.  How might we go looking for problems or confirm this is working correctly?
Flags: qe-verify+
Flags: needinfo?(wkocher)
(In reply to Boris Zbarsky [:bz] from comment #21)
> I don't think a risk assessment of "low" is ever reasonable for docshell.

Reasonable.

> 
> Looking at the patch carefully again, I realized that we missed a case: what
> happens when the docshell goes active before EndPageLoad?

I think that's handled, no? We should hit this[0] and enable the timer.

[0] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7547
Flags: needinfo?(snorp)
(In reply to Boris Zbarsky [:bz] from comment #22)
> Also, seems to me that what you may want to do is avoid doing the refresh,
> not avoid starting the timer, right?  Not entirely clear.

IMHO it's cleaner to avoid starting the timer, since it could fire every second and keep the CPU awake.

> 
> Oh, and while I was reading the diff again, "whenver" is not a word.

Yeah, that's fixed in the version I pushed.
> I think that's handled, no? We should hit this[0] and enable the timer.

No, the problem is that we will enable it immediately when going active instead of waiting for EndPageLoad like we're supposed to, as far as I can tell.
Flags: needinfo?(snorp)
Attachment #8725907 - Flags: approval-mozilla-beta+
Added to Fx47 (beta) release notes.
Doesn't look like there's anything for snorp to do here. Bug 1265769 was filed for the follow-up issue in comment 27.
Flags: needinfo?(snorp)
You need to log in before you can comment on or make changes to this bug.