732368 - Ensure idle service doesn't fire idle events after xpcom-shutdown

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Marco Bonardo [:mak]]

Some test catched this, it should really do nothing after xpcom-shutdown, especially not notify idle-daily

Comment 1

[Marco Bonardo [:mak]]

Actually for many components even  firing this at some profile notification would be bad.  Likely we should stop at profile-change-teardown and xpcom-shutdown to catch both cases.

Comment 2

[Mike Kristoffersen (:MikeK)]

@Marco, do you have a hint, on how to detect these two conditions, then I can disable the idle service on that case - I assume, that once we come to this point, then there is no chance that we will abandon shut-down? I mean, we should not have a condition where we re-enable the Idle service again?

Comment 3

[Marco Bonardo [:mak]]

I think just having the service observe (weakly) those 2 notifications and canceling an eventual timer on them (or whatever else is needed to stop listening to idle) may be enough.
After profile-change-teardown there should be no return, especially once we move to the exit(0) strategy. Once there was the possibility to reinit the profile, we are no more targeting that, though if you want to be on the safe side you may also observe profile-after-change and restart the timer if it was stopped.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to Marco Bonardo [:mak] from comment #3)
> Once there was the possibility to reinit the
> profile, we are no more targeting that, though if you want to be on the safe
> side you may also observe profile-after-change and restart the timer if it
> was stopped.

Fwiw, I /think/ SeaMonkey still allows to switch between profiles...
http://mxr.mozilla.org/comm-central/search?string=profile-after-change&case=on

Comment 5

[Marco Bonardo [:mak]]

from what I got the last time I asked about it, it doesn't exactly that way, it probably restarts the process. Many services still handle that case since nobody removed it, though a lot more don't. Btw, as I said it's easy to stay on the safe side here.

Comment 6

[Marco Bonardo [:mak]]

bumping to major, this causes permanent failures in comm-central

Mike, if you don't plan to work on this shortly, please let us know so someone else may take care of this.

Comment 7

[Mike Kristoffersen (:MikeK)]

Marco, I'm looking into this now.

Comment 8

[Mike Kristoffersen (:MikeK)]

Attachment: (note: doesn't compile) Trying to get the daily idle service to be silent on shut-down

I'm having some difficulties in making the weak reference, I'm sure it is because I don't understand XPCOM completely :)

I get the following linker error when I try to build:

../../widget/xpwidgets/nsIdleService.o:(.data.rel.ro._ZTV18nsIdleServiceDaily[vtable for nsIdleServiceDaily]+0x30): undefined reference to `nsIdleServiceDaily::GetWeakReference(nsIWeakReference**)'
../../widget/xpwidgets/nsIdleService.o:(.data.rel.ro._ZTV18nsIdleServiceDaily[vtable for nsIdleServiceDaily]+0x70): undefined reference to `non-virtual thunk to nsIdleServiceDaily::GetWeakReference(nsIWeakReference**)'
/usr/bin/ld.bfd.real: libxul.so: hidden symbol `nsIdleServiceDaily::GetWeakReference(nsIWeakReference**)' isn't defined
/usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output

Any hints will be appreciated! (or a patch that does what I'm trying to do *S*)

----

I have chosen to make if for the daily idle service, apart from me having problems in adding an observer feature to the idle service, it is much more easy to pause the daily idle class (and restart it) than it would be to do the same for the generic idle service.

If the requirement is for the generic idle service to stop firring idle and non-idle events, then that is of cause also possible, it just requires some more logic.

As I understood the original failure, then it was due to the daily idle timer and not the generic idle timer?

Comment 9

[Justin Lebar (not reading bugmail)]

I'm not sure why you're using a weak ref here, but

+class nsIdleServiceDaily : public nsIObserver,
+                           public nsISupportsWeakReference

should be |public nsSupportsWeakReference|.

https://developer.mozilla.org/en/Weak_reference

Comment 10

[Mike Kristoffersen (:MikeK)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> I'm not sure why you're using a weak ref here, but
> 
> +class nsIdleServiceDaily : public nsIObserver,
> +                           public nsISupportsWeakReference
> 
> should be |public nsSupportsWeakReference|.
> 
> https://developer.mozilla.org/en/Weak_reference

Thank you very much! - now it compiles here - new patch coming up :)

Comment 11

[Mike Kristoffersen (:MikeK)]

Attachment: Disabling the daily idle service on shutdown

@Marco, do you think something like this will do the trick?

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 603066 [details] [diff] [review]
Disabling the daily idle service on shutdown

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

Well, my first thought was to stop any idle event in the service. Stopping idle-daily may be fine for this specific case, though there is still a remote possibility a service may add an idle observer and try to handle the "idle" notification after xpcom-shutdown, before it's being destroyed.  It's mostly an edge-case, what do you think?

::: widget/xpwidgets/nsIdleService.cpp
@@ +96,5 @@
> +    mShutdownInProgress = false;
> +
> +    // Check if we have a pending daily idle
> +    if (mIdleDetectedDuringShutdown) {
> +      mIdleDetectedDuringShutdown = true;

I think you rather wanted to set it to false here.

@@ +97,5 @@
> +
> +    // Check if we have a pending daily idle
> +    if (mIdleDetectedDuringShutdown) {
> +      mIdleDetectedDuringShutdown = true;
> +      return notifyDailyIdle();

hm, this "restart" is complicating the code quite a bit. And it's firing idle-daily on startup of the new profile, that may be a quite noticeable bad perf hit. Things are done on idle to not interrupt the user, that is the opposite of what this is doing.

I think the possibilities of the first meaningful idle firing on shutdown are so small that we are over-engineering that case. Users of this notification already know this may not fire exactly every single day, due to the "randomness" of idle, so forgetting to fire it in rare cases wouldn't be problematic imo. I'd probably remove all the mIdleDetectedDuringShutdown stuff and add a brief comment about why we don't handle that case.

@@ +182,5 @@
> +  // Register for when we should terminate/pause
> +  nsCOMPtr<nsIObserverService> obs
> +    (do_GetService("@mozilla.org/observer-service;1"));
> +
> +  if (obs) {

use mozilla::services::GetObserverService();

::: widget/xpwidgets/nsIdleService.h
@@ +122,5 @@
> +
> +  /**
> +   * Function that is called when the daily idle should be fired
> +   */
> +  nsresult notifyDailyIdle(void);

is passing void a convention of this module?

Comment 13

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

There is a simpler patch at bug 733358. Any comments on it being OK or why it is insufficient would be welcome.

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: alternative patch

From bug 733358

https://tbpl.mozilla.org/?tree=Try&rev=79e1a5dc802c

Comment 16

[Mike Kristoffersen (:MikeK)]

Comment on attachment 603252 [details] [diff] [review]
alternative patch

Marco sugested that we should be listening to both events, that it should be weakly bound, and that we should be able to re-enable it, which is why my patch became a little complicated compared to yours - so he is probably better at judging if this is needed :)

Assuming it is enough to listen on "profile-change-teardown" and we shouldn't be able to recover then your patch is logical similar to what I did and is fine from the idle services point of view, so I'll leave the decision to Marco as he knows more than me about the overall system :)

Comment 17

[Marco Bonardo [:mak]]

until we are at a point where every single app using libxul uses exit(0), we still have to handle the possibility to not have a profile and reach xpcom-shutdown.
On the other side, I think we can simplify Mike's patch as I explained in comment 12, since the case of an idle-daily on shutdown is rare, it's not worth to handling it.

Comment 18

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

(In reply to Marco Bonardo [:mak] from comment #17)
> until we are at a point where every single app using libxul uses exit(0), we
> still have to handle the possibility to not have a profile and reach
> xpcom-shutdown.
> On the other side, I think we can simplify Mike's patch as I explained in
> comment 12, since the case of an idle-daily on shutdown is rare, it's not
> worth to handling it.

I think this stand will unnecessary complicate and slow down the exit(0) work, but the discussion should probably be moved somewhere else. In the interested of speeding this up I will remove my patch and take a look at Mike's.

Comment 19

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: MikeK's patch with Mak's comments

This is MikeK's patch, but I removed the bits about handling an idle event during a "shutdown" that is really just a profile change. An event during a profile change is lost, but this is an uncommon case and not required for correctness.

MikeK, any particular reason why you needed that?

https://tbpl.mozilla.org/?tree=Try&rev=3eee7bbce710

Comment 20

[(dormant account)]

Comment on attachment 603389 [details] [diff] [review]
MikeK's patch with Mak's comments

+  if (!strcmp(aTopic, "profile-after-change")) {
+    // We are back. Start sending notifications again.
+    mShutdownInProgress = false;
+    return NS_OK;
+  }
+
+  if (strcmp(aTopic, "xpcom-will-shutdown") == 0 ||
+      strcmp(aTopic, "profile-change-teardown") == 0) {
+    mShutdownInProgress = true;
+  }

This is weird, should s/profile-change-teardown/profile-before-change/ for symmetry and because the other profile-* notifications are likely to go away

Comment 21

[Marco Bonardo [:mak]]

(In reply to Taras Glek (:taras) from comment #20)
> This is weird, should s/profile-change-teardown/profile-before-change/ for
> symmetry and because the other profile-* notifications are likely to go away

no, that's fine, this must happen before profile-before-change.

Comment 22

[Mike Kristoffersen (:MikeK)]

Comment on attachment 603389 [details] [diff] [review]
MikeK's patch with Mak's comments

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

I can see that it might over complicate the patch to have the restart things if we are coming back from a profile change, and it might not be that important to miss a daily idle in that specific (unlikely) case - but one thing I think we should consider is to restart the timer anyway in that case.

As the patch is now, then if we have a timeout during a profile change then the daily idle will never fire again until the browser is restarted, of cause the profile change is likely to involve user interaction, thous making it even more unlikely...  Is it just me having paranoia or what do others think about this?

Comment 23

[Mike Kristoffersen (:MikeK)]

Besides the above, the patch looks fine to me.

Comment 24

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: alternative patch

This patch sets the timer back on "profile-after-change", but I don't think it is needed. The reason is that with the old patch we would not execute

 // Stop observing idle for today.
  (void)mIdleService->RemoveIdleObserver(this,
                                         DAILY_SIGNIFICANT_IDLE_SERVICE_SEC);

so we would keep being told about "significan idle" events during the day, no?


https://tbpl.mozilla.org/?tree=Try&rev=837b317b2c8b

Comment 25

[Mike Kristoffersen (:MikeK)]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #24)
> Created attachment 603432 [details] [diff] [review]
> alternative patch
> 
> This patch sets the timer back on "profile-after-change", but I don't think
> it is needed. The reason is that with the old patch we would not execute
> 
>  // Stop observing idle for today.
>   (void)mIdleService->RemoveIdleObserver(this,
>                                          DAILY_SIGNIFICANT_IDLE_SERVICE_SEC);
> 
> so we would keep being told about "significan idle" events during the day,
> no?
> 
> 
> https://tbpl.mozilla.org/?tree=Try&rev=837b317b2c8b

Yes, we would, if the user had activity again... which made me think of one thing...  in this very rare case we are talking about, if we don't remove the idle observer, wouldn't we get notified by the idle service when there is user activity and fire the daily idle, as we don't check for the OBSERVER_TOPIC_IDLE string in the observer function - previously this wasn't a problem because we remove our observer and were guarantied to get an OBSERVER_TOPIC_IDLE before an OBSERVER_TOPIC_BACK???  

I'm sorry I didn't notice this before...

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to Marco Bonardo [:mak] from comment #12)
> use mozilla::services::GetObserverService();

Both current patches miss this.


(In reply to Marco Bonardo [:mak] from comment #21)
> (In reply to Taras Glek (:taras) from comment #20)
> > This is weird, should s/profile-change-teardown/profile-before-change/ for
> > symmetry and because the other profile-* notifications are likely to go away
> 
> no, that's fine, this must happen before profile-before-change.

Hum, aren't you replying no and yes at the same time? ;-<

Comment 27

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: new patch

OK, I think this should do it.

We don't remove the observer on "shutdown", so when we get back we are still getting notifications. To make sure we handle only idle messages, I added a strcmp.

https://tbpl.mozilla.org/?tree=Try&rev=fda8eeca7969

Comment 28

[Marco Bonardo [:mak]]

Comment on attachment 603476 [details] [diff] [review]
new patch

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

::: widget/xpwidgets/nsIdleService.cpp
@@ +94,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (strcmp(aTopic, "xpcom-will-shutdown") == 0 ||
> +      strcmp(aTopic, "profile-change-teardown") == 0) {

one check does !strcmp, the others do == 0, please be consistent in the used check

@@ +99,5 @@
> +    mShutdownInProgress = true;
> +  }
> +
> +  if (mShutdownInProgress || strcmp(aTopic, OBSERVER_TOPIC_IDLE) !=0) {
> +    return NS_OK;

just in case, add a MOZ_ASSERT above that the topic at this point is either IDLE or BACK, any other unhandled topic would be a code error.

@@ +169,5 @@
> +
> +  // Register for when we should terminate/pause
> +  nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +
> +  if (obs) {

please remove useless newline

Comment 29

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: new patch

https://tbpl.mozilla.org/?tree=Try&rev=95d796622e8d

Comment 30

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: fixed assert

https://tbpl.mozilla.org/?tree=Try&rev=95e69fce0c05

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 603679 [details] [diff] [review]
fixed assert

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

::: widget/xpwidgets/nsIdleService.cpp
@@ +94,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (strcmp(aTopic, "xpcom-will-shutdown") == 0 ||
> +      strcmp(aTopic, "profile-change-teardown") == 0) {

Marco, could you "confirm" your reply wrt s/profile-change-teardown/profile-before-change/?

@@ +98,5 @@
> +      strcmp(aTopic, "profile-change-teardown") == 0) {
> +    mShutdownInProgress = true;
> +  }
> +
> +  if (mShutdownInProgress || strcmp(aTopic, OBSERVER_TOPIC_BACK) ==0) {

Nit: missing space before '0'.
Fwiw, "!strcmp()" is simpler, but as you prefer.

@@ +100,5 @@
> +  }
> +
> +  if (mShutdownInProgress || strcmp(aTopic, OBSERVER_TOPIC_BACK) ==0) {
> +    return NS_OK;
> +  }

Nit: Insert a blank line ("after" a return).

::: widget/xpwidgets/nsIdleService.h
@@ +119,5 @@
>     */
>    nsCategoryCache<nsIObserver> mCategoryObservers;
> +
> +  /**
> +   * Boolean set to true if we should disable daily idle notifications.

Nit: "Boolean set to true when daily idle notifications should be disabled.".

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 603679 [details] [diff] [review]
fixed assert

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

::: widget/xpwidgets/nsIdleService.cpp
@@ +94,5 @@
> +    return NS_OK;
> +  }
> +
> +  if (strcmp(aTopic, "xpcom-will-shutdown") == 0 ||
> +      strcmp(aTopic, "profile-change-teardown") == 0) {

these 2 notifications are fine considered we don't want to depend on listeners registration.

@@ +98,5 @@
> +      strcmp(aTopic, "profile-change-teardown") == 0) {
> +    mShutdownInProgress = true;
> +  }
> +
> +  if (mShutdownInProgress || strcmp(aTopic, OBSERVER_TOPIC_BACK) ==0) {

what Serge said regarding the missing space, I don't mind about the check, provided it's coherent in all uses.

@@ +101,5 @@
> +
> +  if (mShutdownInProgress || strcmp(aTopic, OBSERVER_TOPIC_BACK) ==0) {
> +    return NS_OK;
> +  }
> +  MOZ_ASSERT(strcmp(aTopic, OBSERVER_TOPIC_IDLE) == 0);

Serge, I don't think we need additional newlines here, the assert is part of this piece of code.

rather I'd have kept the if as it was (more generic != IDLE) as a runtime protection, and moved the assert before it, checking both BACK || IDLE.  But it's not that much important, so whatever.

::: widget/xpwidgets/nsIdleService.h
@@ +119,5 @@
>     */
>    nsCategoryCache<nsIObserver> mCategoryObservers;
> +
> +  /**
> +   * Boolean set to true if we should disable daily idle notifications.

what Serge said, or "Whether shutdown is running and thus daily idle notifications should be disabled."

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to Marco Bonardo [:mak] from comment #32)
> rather I'd have kept the if as it was (more generic != IDLE) as a runtime
> protection, and moved the assert before it, checking both BACK || IDLE.

I would like that ;-)

Comment 34

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I kept the assert after the if. Before the if it would have to check 4 possible messages. Changing the if condition would also not give us protection in a release build, just swap one bug for another.

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=35aa165a23c9

Comment 35

[Marco Bonardo [:mak]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #34)
> I kept the assert after the if. Before the if it would have to check 4
> possible messages.

Ah sorry, I thought you were returning early in the if for the other 2 messages, no problem btw.

Comment 36

[Serge Gautherie (:sgautherie)]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #34)
> Changing the if condition would also not give us
> protection in a release build, just swap one bug for another.

Wouldn't it? Could you explain what would be the other bug?


(In reply to Marco Bonardo [:mak] from comment #35)
> I thought you were returning early in the if for the other 2
> messages

I would support that too.

Comment 37

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Wouldn't it? Could you explain what would be the other bug?

It is a bug for an unexpected message to get there. If it gets there, there is no right answer. We can pretent it is an idle event or not, but the it is the user of this code that would have the bug.

Comment 38

[Serge Gautherie (:sgautherie)]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #37)

> It is a bug for an unexpected message to get there. If it gets there, there

At least, we should not continue and notify is such a case, should we?

> is no right answer. We can pretent it is an idle event or not, but the it is
> the user of this code that would have the bug.

Sure, but shouldn't we just NS_ERROR() or return an error code?
(MOZ_ASSERT() is to help debugging (only).)

Comment 39

[Marco Bonardo [:mak]]

MOZ_ASSERT covers a code error, that is what may happen here, nothing else may happen without the help of a developer, so it's fine.  This is covering really edge cases, so I don't think it's worth discussing, honestly.

Comment 40

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> At least, we should not continue and notify is such a case, should we?

I don't see how that is better or worse than any other behaviour.

> > is no right answer. We can pretent it is an idle event or not, but the it is
> > the user of this code that would have the bug.
> 
> Sure, but shouldn't we just NS_ERROR() or return an error code?
> (MOZ_ASSERT() is to help debugging (only).)

I would be more than happy to use NS_ABORT_IF_FALSE.

Comment 41

[Marco Bonardo [:mak]]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #40)
> I would be more than happy to use NS_ABORT_IF_FALSE.

Not really any better than MOZ_ASSERT, fwiw.

Comment 42

[Mike Kristoffersen (:MikeK)]

Comment on attachment 603679 [details] [diff] [review]
fixed assert

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

I don't have anything new to add about the patch, as long as we will accept that if the profile is changed between "one day since the last idle", and "significant" user activity, then we won't have the daily idle event fired until the user does an active/idle cycle again. I would fix that for correctness, but I accept if it is seen as unneeded.

Comment 43

[Mike Kristoffersen (:MikeK)]

(In reply to Mike Kristoffersen (:MikeK) from comment #42)
> that if the profile is changed between "one day since the last idle", and
> "significant" user activity, 

That should be ""one day since the last idle" and "significant" user _inactivity_"

Comment 44

[Serge Gautherie (:sgautherie)]

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #34)
> https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=35aa165a23c9

https://hg.mozilla.org/mozilla-central/rev/35aa165a23c9