501422 - HTML5 offline resources doesn't work

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Remy Sharp]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.1) Gecko/20090624 Firefox/3.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-GB; rv:1.9.1) Gecko/20090624 Firefox/3.5

When reloading the url after it's supposed to be cached, it returns with:

Content Encoding Error

      

      
      
      

      
        
        

          

The page you are trying to view cannot be shown because it uses an invalid or unsupported form of compression.
------
Content Encoding Error

The page you are trying to view cannot be shown because it uses an invalid or unsupported form of compression.

Please contact the web site owners to inform them of this problem.
-------

I've tried this both with server side gzip enabled *and* disabled as I suspected it might be content decoding, but the bug persists.

Also, when comparing against Safari 4 which does support offline cache, when I reload the URL it *should* hit my server for the manifest file - whereas Firefox 3.5 isn't (though this could be because the resource is completely broken).

Reproducible: Always

Steps to Reproduce:
1. Visit http://html5demos.com/offlineapp
2. Firefox 3.5 requests to store the offline data from the manifest - accept.
3. Reload the page
4. See the error from the details: Problem loading page
5. To start again, clear the offline storage from the preferences.
Actual Results:  
As per details, it should load up the page as it did in the first request.



I've also had another individual set up their server with a similar test to produce the same result:

http://www.carisenda.com/sandbox/storage/

As I said in the details, I do have gzip enabled, but I've also tried this with gzip disabled.

Comment 1

[u88484]

Confirming the behavior but I'm not sure if it is a Firefox bug or not.

Comment 2

[Remy Sharp]

For reference, this exact same test works fine in Safari 4 and the latest iPhone Safari browsers.

Comment 3

[Honza Bambas (:mayhemer)]

Going to take a look at this.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

When the page loads the first time the http://html5demos.com/offlineapp content is stored to the http cache. When user presses the 'Allow' button we start caching (processing the manifest) and the master entry which is the page at http://html5demos.com/offlineapp. Now we load the master entry from the http cache (nsHttpChannel::ReadFromCache). 

Cause of this bug was that we were adding the cache listener sooner then we were applying the content conversion. That way the content conversion stream tee got between the http cache and the offline cache => content were copied from the http cache to the offline cache through the content converter.

Solution is to apply content conversion sooner then we add the offline cache stream tee.

All offline tests are passing.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So without this patch we're writing uncompressed data into the offline cache, basically?  But then expecting to read in compressed data?

I'm not sure I follow the patch, though.. won't this cause us to do the offline cache stuff twice in the ProcessNormal case?

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 387165 [details] [diff] [review]
v1

(In reply to comment #5)
> So without this patch we're writing uncompressed data into the offline cache,
> basically?  But then expecting to read in compressed data?

Exactly

> I'm not sure I follow the patch, though.. won't this cause us to do the offline
> cache stuff twice in the ProcessNormal case?

Yes, that's true. I'll revisit the patch.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v2

This patch moves the offline cache stream creation from ReadFromCache to OnStartRequest. Tests pass and the offline app from URI loads correctly.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Do we not get OnStartRequest called in the non-read-from-cache cases?

Is there a reason to not just move the offline code into CallOnStartRequest?  Will the cache listener stuff that ProcessNormal sets up interfere with that?

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to comment #8)
> Do we not get OnStartRequest called in the non-read-from-cache cases?
> 

Not sure I understand. We get OnStartRequest in both cases - ProcessNormal and ReadFromCache. It's called by the transaction pump OR by the cache pump, in either of cases.

> Is there a reason to not just move the offline code into CallOnStartRequest? 

It would work, but I like it this way more, it's more consistent, initiations are close to each other. But if you insist, I checked the patch is working that way too and we do not duplicate the code.

> Will the cache listener stuff that ProcessNormal sets up interfere with that?

No. ProcessNormal is called from ProcessResponse. When we get there we will not reach my new code in OnStartRequest (there is 'return ProcessResponse();').

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v2 alt [Checkin comment 17]

Alternative version for consideration.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Not sure I understand. We get OnStartRequest in both cases - ProcessNormal and
> ReadFromCache.

In that case, I don't understand how moving code from ReadFromCache to OnStartRequest can possibly be correct unless changes are also made elsewhere or unless the old behavior was just completely wrong...

> No. ProcessNormal is called from ProcessResponse. When we get there we will
> not reach my new code in OnStartRequest (there is 'return
> ProcessResponse();').

Unless this explains things?

But that explanation doesn't answer my question about InstallCacheListener in your v2 alt.

Generally speaking, I prefer either v2 alt or factoring out all that code that's copy/pasted in two different places into a helper method...

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to comment #11)
> > Not sure I understand. We get OnStartRequest in both cases - ProcessNormal and
> > ReadFromCache.
> 
> In that case, I don't understand how moving code from ReadFromCache to
> OnStartRequest can possibly be correct unless changes are also made elsewhere
> or unless the old behavior was just completely wrong...
> 

It was wrong. Offline cache stream tee was installed just after CallOnStartRequest call in ProcessNormal case. That was ok. But in ReadFromCache it was installed BEFORE CallOnStartRequest. That way stream tees switched over -> this bug.

> > No. ProcessNormal is called from ProcessResponse. When we get there we will
> > not reach my new code in OnStartRequest (there is 'return
> > ProcessResponse();').
> 
> Unless this explains things?
> 
> But that explanation doesn't answer my question about InstallCacheListener in
> your v2 alt.

What question?

> 
> Generally speaking, I prefer either v2 alt or factoring out all that code
> that's copy/pasted in two different places into a helper method...

I also like v2 alt. It fixes the code duplication and is a clean solution.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> It was wrong.

That's not what I meant.  It looks like the patch changes not only the ordering, but also the set of cases in which we do the offline cache stuff.  Is the old set or the new set the right set?

> What question?

"Will the cache listener stuff that ProcessNormal sets up interfere with that?"  I haven't looked into whether that inserts things into the data stream as you're doing or not....

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to comment #13)
> > It was wrong.
> 
> That's not what I meant.  It looks like the patch changes not only the
> ordering, but also the set of cases in which we do the offline cache stuff.  Is
> the old set or the new set the right set?

It's not changing the set, just order. 'v2 alt' never installs offline cache listener in another case then before. That's why I added the !mCanceled condition.

> 
> > What question?
> 
> "Will the cache listener stuff that ProcessNormal sets up interfere with that?"
>  I haven't looked into whether that inserts things into the data stream as
> you're doing or not....

Ah, you are probably concerned about fact I switched adding cache and offline cache listeners? I am not aware of any problems, but it probably doesn't answer your question as well. Actually, I have no idea, but all tests are passing and from my knowledge of the code cache listeners just read and don't modify the stream. It's no matter in what order they do it IMO.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> That's why I added the !mCanceled condition.

Ah, ok.  And all the OnStartRequest cases that don't do ProcessNormal did ReadFromCache?

> concerned about fact I switched adding cache and offline cache listeners?

Yes.  I think you're right that it's not a problem.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

Let's do this.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

http://hg.mozilla.org/mozilla-central/rev/f92cd29b14fc

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

This is 1.9.1.x bug, it should actually be even blocking.

Comment 19

[Samuel Sidler (old account; do not CC)]

Can we get a test for this so we ensure it doesn't break again?

Comment 20

[Samuel Sidler (old account; do not CC)]

Comment on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

Not for 1.9.1.2.

Comment 21

[Remy Sharp]

Not sure where my last commment went, but this *isn't* working correctly in the latest Firefox 3.5.2.

The error is different, but the result is the same: HTML 5 offline resource doesn't work.

Here's the issue using the source code from: http://html5demos.com/offlineapp

If you change the manifest version (you'll need to replicate the test yourselves) and refresh the page, the status changes to: UPDATEREADY (4)

Once we refresh the page, the browser correctly requests all the assets from the server (rather than just the manifest).

Now refreshing does one of two things (I've seen two different things happen):

1. The JavaScript assets can't be accessed triggering JS errors (even though they're available), Firebug says there's an error and the buttons don't work.  I've seen this a few times, but the next issue is the main problem:

2. Now the page reloads saying the compression isn't supported (I'm using normal server side gzip):

Content Encoding Error

The page you are trying to view cannot be shown because it uses an invalid or unsupported form of compression.


I suspect some of this bug was resolved and released, but perhaps you're still holding on to the full fix (as per comment #20 ?)

Comment 22

[Honza Bambas (:mayhemer)]

(In reply to comment #21)
> Not sure where my last commment went, but this *isn't* working correctly in the
> latest Firefox 3.5.2.

No it is not. Will probably be part of 3.5.3. The decision has already been made, see comment 20, as you say.

For testing please use latest firefox trunk, http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/

Comment 23

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

Here's a mochitest which covers this failure.  It successfully captures the failure on 3.5, but passes on trunk.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 393276 [details] [diff] [review]
mochitest test case

The test works, detects the bug, good job. However, I would very much like see this test rather put to dom\tests\mochitest\ajax\offline using style of those tests.

If there are some other tests using the offline cache suitable to place to that location as well, then please let me know, we should have a bug to move them all there.

Comment 25

[Jonathan Griffin (:jgriffin)]

Re comment #24, yes there are a couple of other offline cache tests currently at browser/base/content/test.

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 393276 [details] [diff] [review]
mochitest test case

(In reply to comment #25)
> Re comment #24, yes there are a couple of other offline cache tests currently
> at browser/base/content/test.

Ok, then if so, let's move them in a different bug if it makes sense to. Please go forward with your patch.

Comment 27

[Jonathan Griffin (:jgriffin)]

Pushed test as http://hg.mozilla.org/mozilla-central/rev/6f957b8818b9.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

Approved for 1.9.1.4, a=dveditz for release-drivers

Please land the mochitest on the 1.9.1 branch also

Comment 29

[frank goossens]

did some testing with html5 offline using Gecko/20090908 Minefield/3.7a1pre (on win xp) and according to those tests (and my no doubt limited understanding of how the offline appcache should work) it may have been too early to close this bug as it seems too much is stored in the offline cache;

an example on http://futtta.be/html5/offline.php

* offline.php:
** has "no-cache" and "expires" headers set in php
** shows server unix timestamp (so page is different at each request)

* manifest.php:
** declares mozchomp.gif as to be cached ("explicit", using "CACHE:")
** prohibits offline.php from being cached ("online whitelist", using "NETWORK:")

when accessing the page, allowing FF to store data into the offline cache and refreshing the page 2 times, the html-output of offline.php is also stored in the offline cache, as the timestamp does not change any more. 

now please correct me if i'm wrong, but caching offline.php should not happen, (even if it would not have been explicitly excluded in the manifest)?

Comment 30

[Boris Zbarsky [:bzbarsky]]

That sounds like a separate bug.  Could you please file it separately (and comment here with the new bug number)?  Thanks!

Comment 31

[frank goossens]

ok, filed as new bug (https://bugzilla.mozilla.org/show_bug.cgi?id=515353)

Comment 32

[Remy Sharp]

This bug isn't fixed - I'm now getting the exact same error.  It didn't happen for me when I refreshed, as it used to, but when I disable my internet connection, then reload I get the same encoding error as before.

As such I'm suggesting to reopen this bug (see this image for example of connection disabled and encoding error: http://twitpic.com/i1rd9 )

Comment 33

[Nochum Sossonko [:Natch]]

(In reply to comment #32)
Which version of Firefox are you running, this hasn't landed on 3.5.x yet. In any event _something_ was fixed here, with a test to boot, so perhaps a new bug should be filed for the issue you're experiencing.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Yeah, this is fixed.  Given the "refreshed" thing in comment 32, I bet Remy is running 3.5.3.

Comment 35

[Daniel Veditz [:dveditz]]

> fixed
 ... on trunk (and I think in time to make 1.9.2/firefox 3.6)

Not yet fixed on the branch that will become Firefox 3.5.4 (status1.9.1 does not say "fixed") though we are planning on it (blocking1.9.1 says it's blocking the 1.9.1.4 release).

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v2 alt + test merged to 1.9.1 [Checking comment 36]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7a6e7f4ffbe7

Comment 37

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.2 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2a2pre) Gecko/20090921 Namoroka/3.6a2pre. I verified by following the STR in Comment 0. Adding keyword.

Also verified on the trunk using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.3a1pre) Gecko/20090921 Minefield/3.7a1pre.

Comment 38

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.1.4pre) Gecko/20091001 Shiretoko/3.5.4pre