Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Release all OfflineCache custom profile files ASAP after custom profile cache update has finished

VERIFIED FIXED in mozilla16

Status

()

Core
Networking: Cache
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
mozilla16
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-kilimanjaro:?, firefox16-)

Details

(Whiteboard: [blocking-webrtdesktop1+], [Desktop WebRT], [qa!])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
In bug 753990 comment 32 Felipe raised an issue that index.sqlite file created in the custom profile directory cannot be deleted while Firefox process is still running.  

That also means two processes may keep the file open concurrently when the web app bound to that profile is started sooner Firefox is closed.

Thus, we should shut the custom offline cache device ASAP the cache is activated (or discarded) by nsApplicationCache::Active() (or ::Discard()).
Honza, any chance we can get this for FF16? Sorry, I should have poked sooner but was away for vacation.
blocking-kilimanjaro: --- → ?
Whiteboard: [blocking-webrtdesktop1+]
(Assignee)

Comment 2

5 years ago
It's the first bug I'm about to work on now.
We need this fixed for FF 16.
tracking-firefox16: --- → ?

Updated

5 years ago
Whiteboard: [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+], [Desktop WebRT]
(Assignee)

Comment 4

5 years ago
Created attachment 638862 [details] [diff] [review]
v1
Attachment #638862 - Flags: review?(michal.novotny)

Updated

5 years ago
tracking-firefox16: ? → +
Comment on attachment 638862 [details] [diff] [review]
v1

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

::: netwerk/cache/nsDiskCacheDeviceSQL.h
@@ +258,5 @@
>    nsCOMPtr<nsIFile>               mBaseDirectory;
>    nsCOMPtr<nsIFile>               mCacheDirectory;
>    PRUint32                        mCacheCapacity; // in bytes
>    PRInt32                         mDeltaCounter;
> +  bool                            mAutoShutdown;

mAutoShutdown is not initialized in constructor
Attachment #638862 - Flags: review?(michal.novotny) → review+
Blocks: 756620
Honza, Felipe, once this bug is fixed, is there anything that we need to do on the desktop-webapp-installation side to take advantage of the fix. I.e. do we need to add any explicit calls to close the appcache or some such? Or will things "magically work" as soon as this bug is fixed?
(Assignee)

Comment 7

5 years ago
Things should megically work, since I made the code to close the database when the new cache version is either activated (success) or discarded (failure or no-update).  However, manual testing with all integrated would be good.  I can do it, but I don't know what all patches still needs to land and how to test some actual web app installation.
(In reply to Honza Bambas (:mayhemer) from comment #7)
> Things should megically work, since I made the code to close the database
> when the new cache version is either activated (success) or discarded
> (failure or no-update).  However, manual testing with all integrated would
> be good.  I can do it, but I don't know what all patches still needs to land
> and how to test some actual web app installation.

I'll be able to take a look at this (I'm the QA on desktop web apps). When this patch lands, what should I look out for in relation to appcache?
(Assignee)

Comment 9

5 years ago
(In reply to Jason Smith [:jsmith] from comment #8)
> I'll be able to take a look at this (I'm the QA on desktop web apps). When
> this patch lands, what should I look out for in relation to appcache?

First, check a web app loads from app cache (i.e. shut the server down if you can or simply disconnect the machine you test on from internet and load it while Firerox is still open).  Then, if you can, check that after an app has been loaded in the WebRT "foreign" web app's profile Firefox does not any longer keep the file /path/to/just-installed-web-app-profile/OfflineCache/index.sqlite.  That should do.

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 10

5 years ago
Comment on attachment 638862 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/2838ba825479
Attachment #638862 - Flags: checkin+

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Whiteboard: [blocking-webrtdesktop1+], [Desktop WebRT] → [blocking-webrtdesktop1+], [Desktop WebRT], [qa+]
http://mozillamemes.tumblr.com/post/21637966463/yes-i-have-a-condition-that-forces-me-to-insert

One of the patches was causing xpcshell failures. Since I didn't see Try results in any of the bugs, I had to backout the entire push.
https://hg.mozilla.org/integration/mozilla-inbound/rev/981ac887f6e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/925562fd7d68

An example:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13404043&tree=Mozilla-Inbound

TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | running test ...
TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | test failed (with xpcshell return code: 0), see following log:
>>>>>>>

TEST-INFO | (xpcshell/head.js) | test 1 pending

TEST-INFO | (xpcshell/head.js) | test 2 pending

TEST-INFO | (xpcshell/head.js) | test 2 finished

TEST-INFO | (xpcshell/head.js) | running event loop

TEST-INFO | (xpcshell/head.js) | test 2 pending
TEST-INFO | /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js | Starting test_normal

TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/xpcshell/head.js | [Exception... "Cannot modify properties of a WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)"  location: "JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/tests/netwerk/test/unit/test_cacheForOfflineUse_no-store.js :: make_channel_for_offline_use :: line 21"  data: no] - See following stack:
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: do_throw :: line 451
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: _run_next_test :: line 899
JS frame :: /home/cltbld/talos-slave/test/build/xpcshell/head.js :: <TOP_LEVEL> :: line 418

TEST-INFO | (xpcshell/head.js) | exiting test

TEST-INFO | (xpcshell/head.js) | test 2 finished
<<<<<<<
Attachment #638862 - Flags: checkin+
(Assignee)

Comment 12

5 years ago
I forgot to update test_cacheForOfflineUse_no-store.js to use the new API.  It is simple to fix, locally it works.

However, now the patch conflicts with Thinker's patch for bug 769291 that has in the mean time landed on m-i.

The conflicting case happens, however, only when the cache update fails.  In that case the related web app cannot be started anyway.

I will split the patch to two parts:
1. shut the device down after successful activation (update done) ; I will land it today
2. shut the device down after discard of the new cache (no update or update/first load failure) ; will be delayed: I have to test that more deeply because of interthread races and also since that part is not important right now since web apps don't have so far (AFAIK) an update mechanism being run from firefox periodically
(Assignee)

Updated

5 years ago
Blocks: 772919
(Assignee)

Comment 13

5 years ago
Comment on attachment 638862 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a055d955691
Attachment #638862 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/3a055d955691
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Verified on Nightly on Win 7. After install of fabrice's test app for appcache (http://people.mozilla.com/~fdesre/openwebapps/test.html), I cut the internet connection. Then, I launched the app. In the app profile, no folder called OfflineCache existed containing a sqlite file.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-webrtdesktop1+], [Desktop WebRT], [qa+] → [blocking-webrtdesktop1+], [Desktop WebRT], [qa!]
(Assignee)

Comment 16

5 years ago
(In reply to Jason Smith [:jsmith] from comment #15)
> Verified on Nightly on Win 7. After install of fabrice's test app for
> appcache (http://people.mozilla.com/~fdesre/openwebapps/test.html), I cut
> the internet connection. Then, I launched the app. In the app profile, no
> folder called OfflineCache existed containing a sqlite file.

Not sure this is the proper way to verify this bug.  

This bug ensures that after we've successfully cached a web app from Firefox, any file in web app's OfflineCache dir is no longer open by Firefox.  But the files in OfflineCache dir under the web app's profile dir must be present.

> In the app profile, no
> folder called OfflineCache existed containing a sqlite file

If you've seen this behavior then something is wrong.
Oh. Moving back to resolved fixed then. I wonder if I'm looking in the right spot then. I'm looking in:

C:\Users\jsmith\AppData\Roaming\http;people.mozilla.com\Profiles\plcsruws.default
Status: VERIFIED → RESOLVED
Last Resolved: 5 years ago5 years ago
Whiteboard: [blocking-webrtdesktop1+], [Desktop WebRT], [qa!] → [blocking-webrtdesktop1+], [Desktop WebRT], [qa+]
A simple way to verify this is:

- install webapp that has appcache support
- don't close firefox and don't open the app
- try to delete webapp profile (the folder that you pointed above)

you should be able to successfully delete the profile
(Assignee)

Comment 19

5 years ago
(In reply to Felipe Gomes (:felipe) from comment #18)

Thanks, that's it!

> you should be able to successfully delete the profile

On Windows, Linux allows deletion of opened files, AFAIK.  If you are on Linux, you have to check what files Firefox keeps open.
(In reply to Felipe Gomes (:felipe) from comment #18)
> A simple way to verify this is:
> 
> - install webapp that has appcache support
> - don't close firefox and don't open the app
> - try to delete webapp profile (the folder that you pointed above)
> 
> you should be able to successfully delete the profile

Makes sense. Thanks for the pointers. Verified on Windows. I'll test on Linux to be sure.
(In reply to Jason Smith [:jsmith] from comment #20)
> (In reply to Felipe Gomes (:felipe) from comment #18)
> > A simple way to verify this is:
> > 
> > - install webapp that has appcache support
> > - don't close firefox and don't open the app
> > - try to delete webapp profile (the folder that you pointed above)
> > 
> > you should be able to successfully delete the profile
> 
> Makes sense. Thanks for the pointers. Verified on Windows. I'll test on
> Linux to be sure.

Looks good on Linux as well. Running ls -l /proc/<PID> for firefox-bin didn't show anything referencing the profile directory for the app.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-webrtdesktop1+], [Desktop WebRT], [qa+] → [blocking-webrtdesktop1+], [Desktop WebRT], [qa!]
tracking-firefox16: + → -
You need to log in before you can comment on or make changes to this bug.