Closed Bug 517109 Opened 11 years ago Closed 11 years ago

Fast Startup component should restart and issue memory-pressure notification periodically

Categories

(Toolkit Graveyard :: XULRunner, defect)

All
Windows CE
defect
Not set
normal

Tracking

(status1.9.2 beta5-fixed, fennec1.0a3-wm+)

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta5-fixed
fennec 1.0a3-wm+ ---

People

(Reporter: crowderbt, Assigned: crowderbt)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
+++ This bug was initially created as a clone of Bug #509249 +++

I'm not sure what a good interval is for this right now, so I'm going w/ 30 seconds after last-window-close for memory-pressure, and 15 minutes after that for silent restart.  It's possible or even likely that something much less frequent is better, or that we should be basing the heuristic on phone usage itself (ie., only do the expensive/costly browser restart if the phone is long-idle).
Comment on attachment 401137 [details] [diff] [review]
v1

Gavin:  Thoughts on this?
Attachment #401137 - Flags: review?(gavin.sharp)
Duplicate of this bug: 517367
Comment on attachment 401137 [details] [diff] [review]
v1

Shifting review.
Attachment #401137 - Flags: review?(gavin.sharp) → review?(dolske)
Comment on attachment 401137 [details] [diff] [review]
v1

>+const fsRestartEnv = "FASTSTART_INITIATED_RESTART";

Make this caps, RESTART_ENV_VAR or something?

>+function setenv(key, value) {
>+  let env = Cc["@mozilla.org/process/environment;1"].
>+            getService(Components.interfaces.nsIEnvironment);
>+  env.set(key, value);
>+}

I assume the environment bits are working on WinCE/WinMo? ISTR Rob Strong had issues with that when working on the updater...

>+  let _shutdown;

better name: _isShuttingDown (= false).

>       // XXX start our long (15 min?) stub restart timer here

This comment can go away now.

>+      // restart the stub in 15 minutes
>+      _restartTimer = new Timer(restart, 60000 * 15, Ci.nsITimer.TYPE_ONE_SHOT);

I think it would be a bit cleaner to start both timers at the same time. Basically, saying the last window closed, so schedule 2 timers to fire (+30s, +15m). Helps disentagle what's going on, with not waiting for one to fire before starting the 2nd.

And null out _restartTimer in restart(), just to be sure.

>   handle: function fs_handle(cmdLine) {
>     // the rest of this only handles -faststart here
>-    if (cmdLine.handleFlag("faststart-hidden", false))
>+    if (cmdLine.handleFlag("faststart-hidden", false) || (getenv(fsRestartEnv) == "1"))
>       cmdLine.preventDefault = true;

Also call stopMemoryCleanup() here?

There's a period of time between the browser being requested to open a window, and when domwindowopened fires. If the restart timer happens to fire then, it would be rather unfortunate. Kind of edgecasy, but I'd prefer to be paranoid about not quitting in the middle of the user doing something. :)

r+ with these fixes...
Attachment #401137 - Flags: review?(dolske) → review+
Attached patch another spinSplinter Review
(In reply to comment #4)
> I assume the environment bits are working on WinCE/WinMo? ISTR Rob Strong had
> issues with that when working on the updater...

Yes, the shunt preserves environment variables and then passes them to the child process on restart as part of the command-line, so Firefox -> Firefox or Fennec -> Fennec invocations should be okay.  Not sure what problem the updater encounters here.  I'll double check this on WinMo w/ Fennec before I land.

> better name: _isShuttingDown (= false).

The "= false" isn't necessary since undefined will evaluate as false in boolean expressions, but I can add it if you insist.


> >   handle: function fs_handle(cmdLine) {
> >     // the rest of this only handles -faststart here
> >-    if (cmdLine.handleFlag("faststart-hidden", false))
> >+    if (cmdLine.handleFlag("faststart-hidden", false) || (getenv(fsRestartEnv) == "1"))
> >       cmdLine.preventDefault = true;
> 
> Also call stopMemoryCleanup() here?
> 
> There's a period of time between the browser being requested to open a window,
> and when domwindowopened fires. If the restart timer happens to fire then, it
> would be rather unfortunate. Kind of edgecasy, but I'd prefer to be paranoid
> about not quitting in the middle of the user doing something. :)
> 
> r+ with these fixes...

I'm not sure I understand what edge-case you're concerned about here.
Attachment #401137 - Attachment is obsolete: true
Attachment #403335 - Flags: review?(dolske)
(In reply to comment #5)
> Created an attachment (id=403335) [details]
> another spin
> 
> (In reply to comment #4)
> > I assume the environment bits are working on WinCE/WinMo? ISTR Rob Strong had
> > issues with that when working on the updater...
> 
> Yes, the shunt preserves environment variables and then passes them to the
> child process on restart as part of the command-line, so Firefox -> Firefox or
> Fennec -> Fennec invocations should be okay.  Not sure what problem the updater
> encounters here.  I'll double check this on WinMo w/ Fennec before I land.
This was in regards to bug 510836 where the updater was changed to use the shunt's environment.cpp
> This was in regards to bug 510836 where the updater was changed to use the
> shunt's environment.cpp

So this works correctly for you?  Great!
(In reply to comment #5)

> I'm not sure I understand what edge-case you're concerned about here.

The sequence I'm thinking of is:

1) close last window
2) restart gets scheduled... time passes...
3) external app asks browser to open a new URL
4) existing browser process gets request to open new URL
5) restart timer fires before we get around to firing domwindowopened.

I guess you'd actually need to fix this elsewhere, since the existing browser process wouldn't have it's command line handler called again at step 4 (afaik?). So, eh, maybe worth a followup bug.
Attachment #403335 - Flags: review?(dolske) → review+
This component isn't used on Mac OS X at all, unless we've added --enable-faststart to our mozconfigs for those platforms?  I'd be happy to backout, if we still believe this is the cause.
This can't be the cause, since this code isn't even packaged on OS X builds, and is WinCE only.
Dolske, that's what I said too, but I backed it out anyway.  Will try to reland today unless Ts mystically fixed itself after my backout.
beltzner:  Did this Ts regression go away as a result of either of my backouts?
The graph doesn't show any difference, so it wasn't this. Go ahead and reland it. [TBH, the graph is noisy enough that this might not even be a real regression.]
The graph does, in fact, go down on the 22nd. I'm not sure I agree with you, Dolske. Reload the link from comment 9 and expand the time range.
Here's a graphic of the range: http://grab.by/bxo

I guess check it back in and see if it goes up again? I guess it could be poorly timed noise, but that looks like an obvious bump to me.
Attached image Annotated perf graph
The two changesets in question:

Landed:  http://hg.mozilla.org/mozilla-central/rev/899023a9fbb0
Backout: http://hg.mozilla.org/mozilla-central/rev/45a3ded46ebc

This screenshot hilights the duration the patch was in the tree. The apparent increase in Ts begins immediately after the landing time, but persists for about a day after the backout.

The reason I say this might not even be a real regression is that not all of the Talos boxes agree there was a regression. Some went up, some didn't. At least one box went down before the backout, and then back up after the backout. Also, if you look at the period from around October 1st October 10th, there are a number of other instances where a couple of boxes spiked and then dropped, while other boxes did not.
Relanded:
http://hg.mozilla.org/mozilla-central/rev/20d4d539beb1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.