780878 - [ApplicationCache] Manifest file not reloaded

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[De Simone Alessandro]

User Agent: Opera/9.80 (Windows NT 6.1; WOW64; U; en) Presto/2.10.289 Version/12.01

Steps to reproduce:

Hello,

I access a webpage where the attribute manifest is specified on the "html" tag using Firefox 14/15/16.


Actual results:

The first time I go on the webpage, the manifest file is correctly read by Firefox and the content is downloaded.

Then, If I go on the same page again or another webpage specifying the same manifest file, Firefox 14 doesn't load the manifest file at all.

It seems that this problem occurs only when the downloaded content is greater than 50MB (when the "warning" banner appears).


Expected results:

Firefox should always reload the Manifest to see if something has changed.

I used mozregression and I saw that this bug started with nightly 2012-04-01. For all previous versions, it works as expected. 

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-03-31&enddate=2012-04-01

We created a small demo to illustrate this problem. Every file in the Manifest file is 1MB and are sent by the server using this header : "Cache-Control: max-age=31536000, must-revalidate"

The manifest file itself is sent by the server using this header : "Cache-Control: private, max-age=0, must-revalidate". This header should force Firefox to always fetch the version from the server.

Steps to reproduce:

1) Go to http://www.jimmygilles.net/bugzilla/page_a.htm
2) Accept the manifest download
3) Wait for the manifest to be completly downloaded (54 files of 1MB each)
4) The manifest is now ready
5) go to http://www.jimmygilles.net/bugzilla/page_b.htm (which uses the same manifest file)
6) Firefox 14-15-16 : the manifest is not downloaded at all and never will be again

Thank you

Comment 1

[Loic]

According to your regression range, the suspected bug could be:

Bug 674728 - Allow web apps to be "pinned", so that they're guaranteed to be fully cached locally

Comment 2

[De Simone Alessandro]

Hello,

I see you changed the severity to "normal". I can understand that "critical" was a little bit too much (after reading this https://bugzilla.mozilla.org/page.cgi?id=fields.html). In my opinion, we should set the severity to "major" as it completely breaks a big functionality in our website (wwww.coursesmart.com - offline reading).

Thanks!

Alessandro

Comment 3

[Jimmy Gilles]

The good url is http://www.coursesmart.com (with 3 w instead of 4 ;))

Comment 4

[Loic]

(In reply to De Simone Alessandro from comment #2)
> Hello,
> 
> I see you changed the severity to "normal". I can understand that "critical"
> was a little bit too much (after reading this
> https://bugzilla.mozilla.org/page.cgi?id=fields.html).
In general, it's up to devs to choose this field and it's not so important (only for critical/blocker bugs).

Comment 5

[vaneeckhoutnicolas]

Hello,

By checking the attached demo, it seems to be a blocker bug as the manifest is not loaded anymore.

This means that for FF version 14/15/16, the application cache's use is totally broken and won't work for everybody... not only coursesmart.com

As Loic previous comment underlined, the suspect bug could be linked with the "pinned" modifications preventing manifest reload (cf. STATE_OBSOLETE >< STATE_NOUPDATE -> ?)

Thanks for your help,
Nic

Comment 6

[Thinker Li [:sinker]]

Attachment: testcase for this bug

I have wrote a xpcshell testcase for this bug.  It update a manifest two times, and check how many times of fetching the manifest.  It seems working fine for me.

Comment 7

[Jimmy Gilles]

(In reply to Thinker Li [:sinker] from comment #6)
> Created attachment 650830 [details] [diff] [review]
> testcase for this bug
> 
> I have wrote a xpcshell testcase for this bug.  It update a manifest two
> times, and check how many times of fetching the manifest.  It seems working
> fine for me.

Hello,

Did you try the demo as indicated in the first comment ?
Can you reproduce this bug if you test the provided demo manually ?

It seems the problem is reproductible when the total size of the downloaded content is greater than 50 MBytes (a warning banner is displayed by firefox when the content exceeds 50 MBytes).

With the demo, I can reproduce the problem every time I follow the steps as indicated (on firefox 14).

Thank you.
Jimmy

Comment 8

[Thinker Li [:sinker]]

(In reply to Jimmy Gilles from comment #7)
> (In reply to Thinker Li [:sinker] from comment #6)
> > Created attachment 650830 [details] [diff] [review]
> > testcase for this bug
> > 
> > I have wrote a xpcshell testcase for this bug.  It update a manifest two
> > times, and check how many times of fetching the manifest.  It seems working
> > fine for me.
> 
> Hello,
> 
> Did you try the demo as indicated in the first comment ?
> Can you reproduce this bug if you test the provided demo manually ?
> 
> It seems the problem is reproductible when the total size of the downloaded
> content is greater than 50 MBytes (a warning banner is displayed by firefox
> when the content exceeds 50 MBytes).

Every file in your manifest is noly 2k.  I can not reproduce your issue.  Can you check it again?

Comment 9

[Jimmy Gilles]

Attachment: A screenshot of a fiddler2 session.

Hello,

Here is a screenshot of the fiddler2 session.
As you can see, the content of each downloaded file is 1 MByte.
53 files are downloaded, so the total size is greater than 50MBytes.

Thank you !

Comment 10

[Thinker Li [:sinker]]

The wireshark told me your web server send a header with "Content-Length: 2970".  It is exactly what is cached in the cache.  But, your server does not return a Content-Length when I get the file with wget.

Comment 11

[Jimmy Gilles]

(In reply to Thinker Li [:sinker] from comment #10)
> The wireshark told me your web server send a header with "Content-Length:
> 2970".  It is exactly what is cached in the cache.  But, your server does
> not return a Content-Length when I get the file with wget.

Hello,

I tested the download at home and, indeed, I was able to reproduce the same problem as you had... very strange because it was working correctly at work.

So, we made some changes to be sure it works correctly.
- We changed the content of each download file because it was not a valid html. Now, each downloaded file is a valid html (checked with the W3C validator).
- I disabled the gzip compression for those files.

I hope you will be able to reproduce the problem we have :)

Thank you for your help and sorry for this very strange behaviour.

Comment 12

[Jimmy Gilles]

Hello,

Were you (or anybody else) able to reproduce this problem?
Please tell me if I can do something more to help to resolve this issue.

Thank you.

Comment 13

[Thinker Li [:sinker]]

Yes, I can reproduce this problem.  As comment #1 had mentioned pinned offline cache, I have break pointed at nsOfflineCacheUpdateService::Schedule(), but it is never called for exceeding the offline-cache.quota.warn.  I think there seems something wrong before calling into nsOfflineCacheUpdateService.  I will do more test for this bug.

Comment 14

[Jimmy Gilles]

Hello,

I know you must be busy but just a quick question, did you do more tests for this bug? Do you know if this bug can be fixed easily or importants changes must be applied to resolve it?

Thank you very much.

Comment 15

[Jimmy Gilles]

Hello,
Is there someone working on it?
Thanks.

Comment 16

[Thinker Li [:sinker]]

Attachment: Check app permission against ALLOW_NO_WARN

Without this checking, nsOfflineCacheUpdateService don't update
offline cache any more if previous updating exceeds usage and showing
a warning.

@Honza: can you review this?

Comment 17

[Thinker Li [:sinker]]

(In reply to Jimmy Gilles from comment #15)
> Hello,
> Is there someone working on it?
> Thanks.

Sorry! I just returned from a vocation.

Comment 18

[Loic]

Is this bug a regression of bug 674728 (according the changelog in comment #0)? I don't see any bug blocked.

Comment 19

[Thinker Li [:sinker]]

Yes! This bug is a regression of bug 674728.

Comment 20

[Loic]

Thanks for confirming. :)

Comment 21

[Thinker Li [:sinker]]

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

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 664408 [details] [diff] [review]
Check app permission against ALLOW_NO_WARN

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

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +569,1 @@
>          *aAllowed = true;

Hmm.. this has slipped through my review.. good catch!

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> Hmm.. this has slipped through my review.. good catch!

Taking back... Thinker, I think you have landed the last patch in bug 674728 w/o a proper review...

Comment 24

[Thinker Li [:sinker]]

(In reply to Honza Bambas (:mayhemer) from comment #23)
> (In reply to Honza Bambas (:mayhemer) from comment #22)
> > Hmm.. this has slipped through my review.. good catch!
> 
> Taking back... Thinker, I think you have landed the last patch in bug 674728
> w/o a proper review...

I think it is backed out.  https://bugzilla.mozilla.org/show_bug.cgi?id=674728#c112
I still remember it was happening when I am on a vocation.

Comment 25

[Thinker Li [:sinker]]

Commit 21 for test report of try-server.

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #24)
> (In reply to Honza Bambas (:mayhemer) from comment #23)
> > (In reply to Honza Bambas (:mayhemer) from comment #22)
> > > Hmm.. this has slipped through my review.. good catch!
> > 
> > Taking back... Thinker, I think you have landed the last patch in bug 674728
> > w/o a proper review...
> 
> I think it is backed out. 
> https://bugzilla.mozilla.org/show_bug.cgi?id=674728#c112
> I still remember it was happening when I am on a vocation.

I am all time talking about the "Part 1" patch.  You have made some changes that needed one more review round but you didn't ask for it.

Comment 27

[Thinker Li [:sinker]]

(In reply to Honza Bambas (:mayhemer) from comment #26)
> (In reply to Thinker Li [:sinker] from comment #24)
> > (In reply to Honza Bambas (:mayhemer) from comment #23)
> > > (In reply to Honza Bambas (:mayhemer) from comment #22)
> > > > Hmm.. this has slipped through my review.. good catch!
> > > 
> > > Taking back... Thinker, I think you have landed the last patch in bug 674728
> > > w/o a proper review...
> > 
> > I think it is backed out. 
> > https://bugzilla.mozilla.org/show_bug.cgi?id=674728#c112
> > I still remember it was happening when I am on a vocation.
> 
> I am all time talking about the "Part 1" patch.  You have made some changes
> that needed one more review round but you didn't ask for it.

Got it!  I am very sorry!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Should the test be getting checked in too? It doesn't appear it was pushed to Try with the fix.

Comment 29

[Thinker Li [:sinker]]

(In reply to Ryan VanderMeulen from comment #28)
> Should the test be getting checked in too? It doesn't appear it was pushed
> to Try with the fix.

@Ryan, testcase will not be checked in.  The testcase is invalid since mochitest never show warning and checking for data size.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

I assume there's no other way to test this then?

Comment 31

[Thinker Li [:sinker]]

(In reply to Ryan VanderMeulen from comment #30)
> I assume there's no other way to test this then?

I will revise the testcase later with setting ALLOW_NO_WARN by testcase itself.  So, we can simulate the situation after showing warning by the browser.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1da17dd5a92

Comment 33

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #31)
> (In reply to Ryan VanderMeulen from comment #30)
> > I assume there's no other way to test this then?
> 
> I will revise the testcase later with setting ALLOW_NO_WARN by testcase
> itself.  So, we can simulate the situation after showing warning by the
> browser.

You can play with the quota setting in the test.  There are prefs for it, if I recall.  I can help if you need.

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b1da17dd5a92