HTML5 offline resources doesn't work

VERIFIED FIXED

Status

()

defect
--
major
VERIFIED FIXED
10 years ago
a month ago

People

(Reporter: remy, Assigned: mayhemer)

Tracking

({verified1.9.1, verified1.9.2})

1.9.1 Branch
x86
macOS
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta1-fixed, blocking1.9.1 .4+, status1.9.1 .4-fixed)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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.

Updated

10 years ago
Version: unspecified → 3.5 Branch

Comment 1

10 years ago
Confirming the behavior but I'm not sure if it is a Firefox bug or not.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

10 years ago
Component: General → DOM: Mozilla Extensions
Product: Firefox → Core
QA Contact: general → general
Version: 3.5 Branch → 1.9.1 Branch
(Reporter)

Comment 2

10 years ago
For reference, this exact same test works fine in Safari 4 and the latest iPhone Safari browsers.
(Assignee)

Comment 3

10 years ago
Going to take a look at this.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
(Assignee)

Comment 4

10 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #387165 - Flags: review?(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?
(Assignee)

Updated

10 years ago
Attachment #387165 - Attachment is obsolete: true
Attachment #387165 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Posted patch v2 (obsolete) — Splinter Review
This patch moves the offline cache stream creation from ReadFromCache to OnStartRequest. Tests pass and the offline app from URI loads correctly.
Attachment #387649 - Flags: review?(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?
(Assignee)

Comment 9

10 years ago
(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();').
(Assignee)

Comment 10

10 years ago
Alternative version for consideration.
Attachment #387862 - Flags: review?(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...
(Assignee)

Comment 12

10 years ago
(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.
> 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....
(Assignee)

Comment 14

10 years ago
(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.
> 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 on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

Let's do this.
Attachment #387862 - Flags: review?(bzbarsky) → review+
Attachment #387649 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/f92cd29b14fc
Attachment #387862 - Attachment description: v2 alt → v2 alt [Checkin comment 17]
(Assignee)

Comment 18

10 years ago
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.
Attachment #387862 - Flags: approval1.9.1.2?
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Can we get a test for this so we ensure it doesn't break again?
blocking1.9.1: --- → needed
Flags: in-testsuite?
Comment on attachment 387862 [details] [diff] [review]
v2 alt [Checkin comment 17]

Not for 1.9.1.2.
Attachment #387862 - Flags: approval1.9.1.2? → approval1.9.1.3?
(Reporter)

Comment 21

10 years ago
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 ?)
(Assignee)

Comment 22

10 years ago
(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/
(Assignee)

Updated

10 years ago
Attachment #387649 - Attachment is obsolete: true
Here's a mochitest which covers this failure.  It successfully captures the failure on 3.5, but passes on trunk.
Attachment #393276 - Flags: review?(hanzlicekk)
Attachment #393276 - Flags: review?(hanzlicekk) → review?(honzab.moz)
(Assignee)

Comment 24

10 years ago
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.
Attachment #393276 - Flags: review?(honzab.moz) → review-
Re comment #24, yes there are a couple of other offline cache tests currently at browser/base/content/test.
(Assignee)

Comment 26

10 years ago
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.
Attachment #393276 - Flags: review- → review+
Pushed test as http://hg.mozilla.org/mozilla-central/rev/6f957b8818b9.
Flags: in-testsuite? → in-testsuite+
blocking1.9.1: needed → .4+
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
Attachment #387862 - Flags: approval1.9.1.3? → approval1.9.1.4+

Comment 29

10 years ago
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)?
That sounds like a separate bug.  Could you please file it separately (and comment here with the new bug number)?  Thanks!
(Reporter)

Comment 32

10 years ago
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 )
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Yeah, this is fixed.  Given the "refreshed" thing in comment 32, I bet Remy is running 3.5.3.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
> 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).
(Assignee)

Updated

10 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
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
Keywords: verified1.9.1
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.