Closed Bug 793970 Opened 7 years ago Closed 7 years ago

Reuse nsAppStartup's watchdog to compulsively power-off/reboot/quit gecko if profile synchronizing hangs.

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: airpingu, Assigned: airpingu)

References

Details

Attachments

(1 file, 2 obsolete files)

We need to use a watchdog thread with timer to force gecko to power off or reboot the device if the profile synchronizing hangs. Some codes can be refactored and reused from nsAppStartup.cpp.
This should be a basecamp-blocker because it blocks Bug 790527 which is also a basecamp-blocker.
Blocks: 790527
blocking-basecamp: --- → ?
Hi :cjones,

I'd like to take this one if you don't mind. I believe most of the codes can be shared from the codes you made in nsAppStartup.cpp. Probabaly, make it a generic service? What do you suggest?
Please do!

The best way to implement this may be to
 - a restart() method to the PowerManagerService
 - change b2g chrome to use this restart() method instead of nsIAppStartup.quit()
 - re-use your implementation of profile sync to implement Restart(), but _exit(0) after the process finishes
 - then move the watchdog code we added to nsAppStartup.cpp out of there and into PowerManagerService, and pass it a parameter letting it to know what to do when the watchdog timer expires (poweroff vs. reboot vs. quit)

Does that make sense?
blocking-basecamp: ? → +
Sounds great! Should be able to come back with the patch tonight or tomorrow. Thanks for the direction!
Assignee: nobody → clian
(In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> Please do!
> 
> The best way to implement this may be to
>  - a restart() method to the PowerManagerService
>  - change b2g chrome to use this restart() method instead of
> nsIAppStartup.quit()

One quick question: is the point you mentioned to replace nsIAppStartup.quit() with PowerManagerService.restart() at http://mxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.js#190?

>  - re-use your implementation of profile sync to implement Restart(), but
> _exit(0) after the process finishes
>  - then move the watchdog code we added to nsAppStartup.cpp out of there and
> into PowerManagerService, and pass it a parameter letting it to know what to
> do when the watchdog timer expires (poweroff vs. reboot vs. quit)
> 
> Does that make sense?
(In reply to Gene Lian [:gene] from comment #5)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #3)
> > Please do!
> > 
> > The best way to implement this may be to
> >  - a restart() method to the PowerManagerService
> >  - change b2g chrome to use this restart() method instead of
> > nsIAppStartup.quit()
> 
> One quick question: is the point you mentioned to replace
> nsIAppStartup.quit() with PowerManagerService.restart() at
> http://mxr.mozilla.org/mozilla-central/source/b2g/components/UpdatePrompt.
> js#190?
> 

That's correct.  Then we can keep all this code in one place.
And it would allow the gaia system app to request a b2g restart, which is useful for testing.
Got it! Thanks Chris! :)
Summary: Use a watchdog thread to force gecko to *really* power off or reboot if the profile synchronizing hangs. → Reuse nsAppStartup's watchdog to compulsively power-off/reboot/quit gecko if profile synchronizing hangs.
Attached patch WIP Patch (obsolete) — Splinter Review
Hi :cjones :)

I've done a quick WIP patch. Could you please take a look and let me know if I'm on the right direction? I'll refine the codes and test them at the same time. Thank you very much!

One question: how to deal the ANDROID platform when calling pmService.restart()?
Attachment #665278 - Flags: feedback?(jones.chris.g)
Comment on attachment 665278 [details] [diff] [review]
WIP Patch

>diff --git a/b2g/components/UpdatePrompt.js b/b2g/components/UpdatePrompt.js

> #ifndef ANDROID

Let's use the existing nsIAppService here for non-android (which
should be MOZ_WIDGET_GONK) ...

> #else

and use the nsIPowerManagerService interface for MOZ_WIDGET_GONK.

> #endif

The rest looks just fine.  Nice work!
Attachment #665278 - Flags: feedback?(jones.chris.g) → feedback+
(In reply to Chris Jones [:cjones] [:warhammer] from comment #10)
> Comment on attachment 665278 [details] [diff] [review]
> WIP Patch
> 
> >diff --git a/b2g/components/UpdatePrompt.js b/b2g/components/UpdatePrompt.js
> 
> > #ifndef ANDROID
> 
> Let's use the existing nsIAppService here for non-android (which
> should be MOZ_WIDGET_GONK) ...
> 

Sorry, this was unclear.  What I meant was, this ifdef should really be

#ifndef MOZ_WIDGET_GONK
Attached patch Patch (obsolete) — Splinter Review
Hi Cjones,

The formal patch is done. One noticeable change is we need to carefully release the parameter pointer passed to pthread. Since the QuitHard() is considered, it's kind of hard to use the auto pointers; instead, we have to release it manually. Not sure if we could have other fancy ways to do that or it's just OK to you. ;)

Btw, to test the performance I added some dummy codes like |while(1) {};| to simulate the hanging behaviors when shutting down. The watchdog can terminate everything as expected if timeout.

Thanks for your review again! :)
Attachment #665278 - Attachment is obsolete: true
Attachment #666503 - Flags: review?(jones.chris.g)
Comment on attachment 666503 [details] [diff] [review]
Patch

>diff --git a/dom/power/PowerManagerService.cpp b/dom/power/PowerManagerService.cpp

> NS_IMETHODIMP
>+PowerManagerService::Restart()
>+{
>+  StartForceQuitWatchdog(eHalShutdownMode_Restart, mWatchdogTimeoutSecs);
>+  // To synchronize any unsaved user data before restarting.
>+  SyncProfile();
>+  _exit(0);

Leave a comment here that this implementation is currently
gonk-specific, because it relies on the gonk init process to restart
b2g.  Let's file a followup bug on fixing this and add a FIXME/bug
XXXXXX comment here.

>diff --git a/dom/power/nsIPowerManagerService.idl b/dom/power/nsIPowerManagerService.idl

>   void              powerOff();
>   void              reboot();
>+  void              restart();

Document here that this interface restarts the Gecko processes, as
opposed to restarting the machine.

Looks good!  r=me with those changes.
Attachment #666503 - Flags: review?(jones.chris.g) → review+
Blocks: 796826
Attached patch Patch 1.1Splinter Review
Addressing comment #13 and r=cjones.

Thanks for the reviews!
Attachment #666503 - Attachment is obsolete: true
Attachment #666880 - Flags: review+
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6bcb5f9359d8
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.