795887 - Flush disk cache when Metro Firefox gets suspended

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Brian R. Bondy [:bbondy]]

+++ This bug was initially created as a clone of Bug #745075 +++

Currently we have 25% disk cache corruption on Metro Firefox. 

Bug 105843 tells the story of how the Necko disk cache is "corrupted" when the app crashes. On Metro Firefox, we can be killed by the OS when the app is suspended.  The app is suspended anytime it is not the currently in use Metro application.

We should flush the disk cache on a suspend notifications.

This 25% will be greatly improved by the work I'm doing with bug 105843, but this should be done independent of that task.

Comment 1

[Brian R. Bondy [:bbondy]]

So now that we have more users, the real stat here is 52% of startups have to nuke the old cache and start over.  We can monitor telemetry data after this fix lands as well to see if we need to keep bug 105843 on the MVP list.

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Adding process suspend observers where sleep ones exist

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Adding process suspend observers where sleep ones exist (Same as other patch but this is for elm)

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Path v1 - Add process suspend / resume handlers to shutdown and resume cache

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Metro changes to sync with android observer

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Remove Android netwerk cache code from widget since it's handled in netwerk/cache now

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 676888 [details] [diff] [review]
Patch v1 - Remove Android netwerk cache code from widget since it's handled in netwerk/cache now

Looks good to me, but Brad is a better reviewer in widget.

Comment 8

[Benjamin Smedberg]

Comment on attachment 676884 [details] [diff] [review]
Patch v1 - Adding process suspend observers where sleep ones exist

What is the difference between the existing sleep_notification and application-background? Are these documented anywhere?

Comment 9

[Brian R. Bondy [:bbondy]]

sleep_notification and wake_notification are documented here:
https://developer.mozilla.org/en-US/docs/Observer_Notifications

application-background and application-foreground are sent when a process is suspended, but not the whole computer.  

If this patch is r+'ed I'll update the documentation in the link above to add the 2 foreground/background ones. They should be added anyway since they already exist in mobile.

Comment 10

[Benjamin Smedberg]

If they aren't documented in-code, they might as well not exist: MDN is a useless waste of time. Right now I don't see any particular reason for those to be separate notifications: are there any actual differences in behavior between the two? If not, can we just have a single notification which covers both cases?

Comment 11

[Brian R. Bondy [:bbondy]]

They are 2 different events and I see value in having them distinct.  Their frequency differs greatly.

Firefox doesn't currently, but an application may at some point want to do an operation differently depending on if the computer is going to sleep or not.

Firefox on Metro's process suspend notification will be sent every time a user switches out of the Metro browser for more than a few seconds.  This may happen 50 times a day for example.  The sleep notification will only happen when the computer goes to sleep which may happen for example once a day.

Comment 12

[Brian R. Bondy [:bbondy]]

(In reply to Brian R. Bondy [:bbondy] from comment #11)
> They are 2 different events and I see value in having them distinct.  Their
> frequency differs greatly.
> 
> Firefox doesn't currently, but an application may at some point want to do
> an operation differently depending on if the computer is going to sleep or
> not.
> 
> Firefox on Metro's process suspend notification will be sent every time a
> user switches out of the Metro browser for more than a few seconds.  This
> may happen 50 times a day for example.  The sleep notification will only
> happen when the computer goes to sleep which may happen for example once a
> day.

Actually if these patches land there would be 1 differentiating factor between the 2 notifications.

When Metro and Android suspend a process, it may be killed at any time for memory reasons. But when a computer goes to sleep, there is not much risk of the computer not waking up. 

So the netwerk patch in this bug will flush the cache only in the process suspended case.

Comment 13

[Brian R. Bondy [:bbondy]]

bsmedberg, I provided the reasons why I think it's better to have 2 notifications, but if you'd like I can consolidate the notifications into one, and I won't fight on it.  It's mostly important to me just to get this done.

Comment 14

[Benjamin Smedberg]

I think it's ok to have two if the behavior is actually going to be different. I just want to insist that they are documented in an appropriate header file.

Comment 15

[Brian R. Bondy [:bbondy]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #14)
> I think it's ok to have two if the behavior is actually going to be
> different.

Yup I gave the reasoning about why and how the behavior is different above.

> I just want to insist that they are documented in an appropriate
> header file.

Is adding an explanatory comment to the existing places where we fire this from within widget/android and widget/win32 (source files) good enough?  Otherwise which header should I define these observers in?  I did a search for a few existing observers that I know about but didn't find a header file that contains all of observer topics.

Comment 16

[Benjamin Smedberg]

nsXPCOM.h is a pretty good example. There isn't nor should there be a single header with all the observer topics listed. If these topics are primarily "widget-level" topics then I would expect them to be documented in some header in widget/. And no, I don't think documenting them at the site they are fired is good, because if/when they are added to other platforms, there needs to be a single site which explains the behavior.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - Define sleep/suspend observers in widget

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 677532 [details] [diff] [review]
Patch v1 - Define sleep/suspend observers in widget

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

r+ with that

::: widget/nsIWidget.h
@@ +161,5 @@
>  
>  /**
> + * Before the OS goes to sleep, this topic is notified.
> + */
> +#define NS_WIDGET_SLEEP_OBSERVER_ID "sleep_notification"

call these TOPIC instead of ID

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Define sleep/suspend observers in widget

:%s/_ID/_TOPIC/g
Carrying forward r+.

Comment 20

[Michal Novotny [:michal]]

Comment on attachment 676886 [details] [diff] [review]
Path v1 - Add process suspend / resume handlers to shutdown and resume cache

r+ with following nits fixed:


>          // Main thread may have already called nsCacheService::Shutdown
> -        if (!nsCacheService::gService || !nsCacheService::gService->mObserver)
> +        if (!nsCacheService::gService || !nsCacheService::IsInitialized())
>              return NS_ERROR_NOT_AVAILABLE;

I would prefer to move the check of nsCacheService::gService into nsCacheService::IsInitialized().


> -    if (gService->mObserver)
> +    if (nsCacheService::IsInitialized())
>          gService->mEnableDiskDevice = gService->mObserver->DiskCacheEnabled();

This check is not necessary. nsCacheService::SetDiskCacheCapacity() is called only from observer and from nsSetSmartSizeEvent::Run() where the check already is.

Comment 21

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Add process suspend / resume handlers to shutdown and resume cache

Implemented review nits. Carrying forward r+.

Comment 22

[Brian R. Bondy [:bbondy]]

This is causing some Android tests to fail. I don't have the setup or time currently to investigate more, but here is the tbl logs: https://tbpl.mozilla.org/?tree=Try&rev=00c52b62a5de

I'm going to just leave the Android code as it was and rename the notification for metro to process_suspend_notification.  If we want to consolidate the Android code later and if that is a good idea, we can do that in a follow up.

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Netwerk

Same as before just renamed observer topic to suspend_process_notification. Carrying forward r+.

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Patch v3 - Widget

Same as before, just renamed observer topic. Carrying forward r+.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - Other m-c code

Renamed observer topic.  Carrying forward r+.

Comment 26

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6e70f8b8b025
http://hg.mozilla.org/integration/mozilla-inbound/rev/479a7ef74ae0
http://hg.mozilla.org/integration/mozilla-inbound/rev/ea5c4c1b0edf

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6e70f8b8b025
https://hg.mozilla.org/mozilla-central/rev/479a7ef74ae0
https://hg.mozilla.org/mozilla-central/rev/ea5c4c1b0edf