Closed Bug 651441 Opened 13 years ago Closed 13 years ago

HTML5 application cache doesn't work when there's no manifest attribute (even if cache previously initialized)

Categories

(Core :: DOM: HTML Parser, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6
Tracking Status
firefox5 + fixed

People

(Reporter: jimmygilles, Assigned: hsivonen)

References

Details

Attachments

(4 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

Hi guys,

In Firefox 4.0, I have a bug that was not present in firefox 3.6. So, it seems it is a regression.

The problem concerns the HTML5 applicationCache (manifest).
This problem is very important to me because this is the core of a new functionality.
And this new functionality will be launch on Firefox at the beginning.

Sorry, I can't show you a demo because the changes are not yet released.
I hope my explanations will help you.

So...

When a user is logged-in in the website, he can decide to download some html pages to the applicationCache because he wants to read those pages in offline.

To do this, he goes to a special page, selects the pages he wants to read and clicks on a button "download".
This action opens a popup. In the HTML of this popup I set the attribute "manifest" on the "html" tag.
Firefox detects this attribute, downloads the manifest and downloads all entries in the manifest.
The manifest contains a list of html pages, css, javascript and images.
The download complete successfully.

If I go to "about:cache" => "Offline cache device", I can see all downloaded entries.
I can switch offline, I can go to my (downloaded) html pages and they are rendering correctly (with CSS, etc.)
That works fine.

Now, if I clear the "classic" cache (clear cache), I have a problem (I am still working offline - no connection to the network).
The applicationCache is still there. I can see it in the list at the bottom (in the preferences window -> advanced -> network), and it takes about 5 MB.

If I return to my html pages, Firefox cannot reache the CSS/JS that are in the applicationCache (I don't know why).
So the pages are not rendering correctly.

I can still see all entries in "about:cache" => "Offline cache device", and if I copy the URL of the CSS referenced by my html pages
I can open it without any problem; idem for javascript, images, etc.
All URLs are corrects, the ones that are referenced in my HTML pages are exactly the same than the ones in the manifest.

In firefox 3.6 it worked very fine!

Please, can you tell my if this is a bug on your side asap? (and if yes, if you can fix it quickly :))
Thank you very much!

Jimmy Gilles


Reproducible: Always

Steps to Reproduce:
See details.
Actual Results:  
The CSS/JS cannot be reached

Expected Results:  
The CSS/JS can be reached
I cannot reproduce the issue.

Does it still occur if you start Firefox in Safe Mode?
http://support.mozilla.com/en-US/kb/Safe+Mode

How about with a new, empty profile?
http://support.mozilla.com/en-US/kb/Basic+Troubleshooting#Make_a_new_profile
Hello,

I can reproduce the issue in safe mode.
I did not test it with a new profile but I have a new installation of Firefox 4.0

Here are some steps you can try to reproduce.

You have :
page_a.html - In this page, set the "manifest" attribute

The manifest contains:
page_b.html (without the manifest attribute)
mycss.css
myjs.js
myimage.gif

page_b.html has a reference to mycss.css, myjs.js and myimage.gif (with <link .../>, <script ... />, <img ... />)

Once the pages are downloaded, switch offline and clear you cache (not the "applicationCache").
Now, go to "page_b.html", the CSS, javascript and image cannot be downloaded by the page.

Thanks.
A reduced Testcase attached to this Report would be nice.
https://bugzilla.mozilla.org/attachment.cgi?bugid=651441&action=enter
Keywords: testcase-wanted
Version: unspecified → 4.0 Branch
Attached file A demo
Hello,

Here is a demo.
I have uploaded the demo on this web-site:
http://www.maquinasindustriales.org/bugzilla/page_a.html

Once the manifest has been downloaded:
- go to "page_b.html"
- Switch offline
- everything is fine
- clear your cache (not the applicationCache).
- the CSS and JS are not found.
Thanks for that Demo Site!
Seeing this too now.
If you keep refreshing the Site whilst being in Offline Mode you see the yellow Background appear and disappear within Milliseconds.

No Idea if this is intended or not.
Maybe a Regression Range would help...
http://harthur.github.com/mozregression/
Severity: major → normal
Hello,

Thank you for this tool! It helped me to find a regression range.

Here is the result:

Last good nightly: 2010-05-03 First bad nightly: 2010-05-04
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=d6bb0f9e9519
In that Range the HTML5 Parser got enabled by default.
And indeed, setting html5.parser.enable;false does not show the Issue.
Component: General → HTML: Parser
Product: Firefox → Core
QA Contact: general → parser
Version: 4.0 Branch → Trunk
Hmm.  Does the problem disappear if you switch the order of the script and stylesheet in page_b.html ?
Henri, can you take a look?
Status: UNCONFIRMED → NEW
Ever confirmed: true
No, it does not resolve the problem; you can test it on page_c.html.
AFAICT, the parser calls into the manifest processing code as expected. Honza, can you figure out what's wrong here?
-> me

Going to analyze.
Assignee: nobody → honzab.moz
First clue: parser doesn't call ProcessOfflineManifest for page_b, a method that is responsible for joining the document with the application cache the page (page_b.html in our case) has been loaded from.  Sub-loads then queries the document (through callbacks) for the application cache, but it is null when STR are used - we try to load it from HTTP cache but cache's been deleted - we fail.

It is not called even when the cache was not deleted.

We are apparently missing a test for this.

My knowledge of the parser code is not that good to continue analyzing this, but if there is no one else to this, let me know.
We definitely need mochitests for this scenario, too!
Assignee: honzab.moz → hsivonen
Status: NEW → ASSIGNED
Nominating as a tracked bug, since this is a pretty bad regression.
Summary: Problem with the Html5 applicationCache. CSS and JS cannot be found. → HTML5 application cache doesn't work when there's no manifest attribute (even if cache previously initialized)
Honza, is http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/test_offlineNotification.html the best application cache mochitest to learn from?

It seems to have more things going on than what's required to test this case. Any advice how to best write a mochitest for this?
Attachment #529688 - Flags: review?(honzab.moz)
(In reply to comment #16)
> Honza, is...?

It is not a bad example.  Also be aware of tests in /dom/tests/mochitest/ajax/offline/.  There is also a test that directly tests offline mode.  Those tests are very deep but have to be rewritten to not use universal connect, so be aware.  So, I would suggest (just a first thought):

- have a main test page
- load a page that is more or less what page_a.html does
- then turn to offline mode
- wipe the cache (entries you need or everything)
- load a page that is more or less what page_b.html does
- I think the best to check the subcontent has been loaded is through a js script that sets some variable or changes some content on the page_b, but that is of course up to you ; AFAICT css and script loads are affected the same way by this bug

The reason to have a top page is to make the test easier to migrate to the new way of writing a privileged tests.  See also bug 582472 for how you can write the test correctly.
Comment on attachment 529688 [details] [diff] [review]
ProcessOfflineManifest even when there's no manifest attribute

r=honzab.

For me this works.
Attachment #529688 - Flags: review?(honzab.moz) → review+
tracking for firefox 5 to see how the landing on the trunk pans out. if all goes well and there's a desire to get it into the branch (there seems to be,) please nominate the patch.
Attached patch Mochitest (obsolete) — Splinter Review
This test was much harder to write than the fix...
Attachment #529973 - Flags: review?(honzab.moz)
OS: Windows 7 → All
Hardware: x86_64 → All
Comment on attachment 529973 [details] [diff] [review]
Mochitest

Review of attachment 529973 [details] [diff] [review]:

This really looks good!  Thanks.

r=honzab for the changes except those in specialpowers.js.

Adding Clint for review.  Clint, can you please take a look at the SpecialPowers changes?

::: parser/htmlparser/tests/mochitest/file_651441.cacheManifest
@@ +1,2 @@
+CACHE MANIFEST
+CACHE:

You don't need this line, it is implicit

::: parser/htmlparser/tests/mochitest/test_651441.html
@@ +17,5 @@
+</div>
+<pre id="test">
+<script type="application/javascript">
+
+/** Test for Bug 651441 **/

Also some simple list of actions this test is doing, one by one, would be good for people that will try to quickly understand it:

This test:
- loads a page referring a page with a manifest
- the manifest caches here and there
- then browser goes offline
- etc...

@@ +23,5 @@
+SimpleTest.waitForExplicitFinish();
+
+var w = window.open("file_651441-1.html");
+
+function startTest() {

Add a comment this is called from onload of file_651441-1.html.

@@ +27,5 @@
+function startTest() {
+  SpecialPowers.pressNotificationButton(w, "offline-app-requested-mochi.test", 0);
+}
+
+function goOffline() {

Add a comment this method is called after the app-cache update invoked by the first file has finished.

@@ +43,5 @@
+function navigateWindow() {
+  w.location = "file_651441-2.html";
+}
+
+function finish() {

Add a comment that this, if the test was successful, is called by a script that is referred by file_651441-2.html.

@@ +45,5 @@
+}
+
+function finish() {
+  ok(true, "The script from app cache should have run.");
+  w.close();

When the test fails this will never get called.  Is mochitest capable of closing (i.e. cleaning up) this window/tab?

@@ +46,5 @@
+
+function finish() {
+  ok(true, "The script from app cache should have run.");
+  w.close();
+  SpecialPowers.setOfflineMode(false);

The same here, will this get cleaned up?
Attachment #529973 - Flags: review?(honzab.moz)
Attachment #529973 - Flags: review?(cmtalbert)
Attachment #529973 - Flags: review+
(In reply to comment #21)
> Comment on attachment 529973 [details] [diff] [review] [review]
> Mochitest
> 
> Review of attachment 529973 [details] [diff] [review] [review]:
> 
> This really looks good!  Thanks.
> 
> r=honzab for the changes except those in specialpowers.js.

Thanks.

> ::: parser/htmlparser/tests/mochitest/file_651441.cacheManifest
> @@ +1,2 @@
> +CACHE MANIFEST
> +CACHE:
> 
> You don't need this line, it is implicit

Removed.

> ::: parser/htmlparser/tests/mochitest/test_651441.html
> @@ +17,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +
> +/** Test for Bug 651441 **/
> 
> Also some simple list of actions this test is doing, one by one, would be
> good for people that will try to quickly understand it:
> 
> This test:
> - loads a page referring a page with a manifest
> - the manifest caches here and there
> - then browser goes offline
> - etc...

Added.

> @@ +23,5 @@
> +SimpleTest.waitForExplicitFinish();
> +
> +var w = window.open("file_651441-1.html");
> +
> +function startTest() {
> 
> Add a comment this is called from onload of file_651441-1.html.

Added.

> @@ +27,5 @@
> +function startTest() {
> +  SpecialPowers.pressNotificationButton(w,
> "offline-app-requested-mochi.test", 0);
> +}
> +
> +function goOffline() {
> 
> Add a comment this method is called after the app-cache update invoked by
> the first file has finished.

Added.

> @@ +43,5 @@
> +function navigateWindow() {
> +  w.location = "file_651441-2.html";
> +}
> +
> +function finish() {
> 
> Add a comment that this, if the test was successful, is called by a script
> that is referred by file_651441-2.html.

Added.

> @@ +45,5 @@
> +}
> +
> +function finish() {
> +  ok(true, "The script from app cache should have run.");
> +  w.close();
> 
> When the test fails this will never get called.  Is mochitest capable of
> closing (i.e. cleaning up) this window/tab?

Not sure, but we have plenty of tests with this sort of failure mode, so I think this should be OK.

Moving forward the request for Clint to review the SpecialPowers bits.
Attachment #529973 - Attachment is obsolete: true
Attachment #529973 - Flags: review?(cmtalbert)
Attachment #530599 - Flags: review?(cmtalbert)
Attachment #530599 - Flags: review?(cmtalbert) → review?(ctalbert)
If we don't manage to get this for 5, we'll re-nominate at 6. Unsetting nom for now.
Comment on attachment 530599 [details] [diff] [review]
Mochitest with Honza's comments addressed

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

Overall, the patch looks good.  I have two main concerns:
1. This won't work on Fennec (see below)
2. The failure mode here is pretty bad.  If this test fails, it will fail by timing out (never calling simpletest.finish()) and it will leave us in an offline state with an extra window running around, which will likely break most of the mochitests that follow it.  I'd prefer that if the test fails, it fails explicitly, and I've suggested a couple of ideas below to help you make it a little more explicit.  These ideas won't negate the danger of the test timing out, but they will at least ensure we get something in the log if things start to misbehave.

r+ with the changes

::: parser/htmlparser/tests/mochitest/test_651441.html
@@ +42,5 @@
> +
> +// Called from the onload handler in file_651441-1.html
> +function startTest() {
> +  SpecialPowers.pressNotificationButton(w, "offline-app-requested-mochi.test", 0);
> +}

What's the failure mode here?  If something happens and the onload handler in file_651441-1.html gets processed before the inline script is activated then this could be called without a notification window present. We need to handle that error and fail the test (probably should call SimpleTest.finish() at that point after failing the test.

@@ +59,5 @@
> +}
> +
> +function navigateWindow() {
> +  w.location = "file_651441-2.html";
> +}

These three functions (goOffline, wipeCache, and navigateWindow) depend on being called sequentially but do not enforce the fact that they are called sequentially.  If they could detect that they are being called out of order, then they could fail the test and ensure we cleanup properly.  I'm harping on cleaning up because if this test silently manages to fail and leave us with an open window and in an offline state, we're going to blow the rest of the testrun.

@@ +67,5 @@
> +  ok(true, "The script from app cache should have run.");
> +  w.close();
> +  SpecialPowers.setOfflineMode(false);
> +  SimpleTest.finish();  
> +}

The finish function is good.  But I'd factor out your finish and cleanup.  

function finish() {
  ok (true, "script ran");
  cleanup();
}

function cleanup(){
  w.close();
  SpecialPowers.setOfflineMode(false);
  SimpleTest.finish();
}
This way, if the click on the notification button fails you can still call cleanup and not hork the rest of the test run.

::: testing/mochitest/specialpowers/content/specialpowers.js
@@ +208,5 @@
> +      .getNotificationBox()
> +      .getNotificationWithValue(notificationValue)
> +      .childNodes[buttonIndex]
> +      .click();
> +  },

This needs to handle the case where the notification window doesn't exist and return some kind of indication that it failed.

Also, this code won't work in fennec, because fennec has no gBrowser.  So for fennec, you'll need to call getNotificationBox (http://mxr.mozilla.org/mozilla-central/ident?i=getNotificationBox) API, then you can navigate through the DOM to get the button you want to click.

You call the this API from the chrome window: window.getNotificationBox(aBrowser);
Attachment #530599 - Flags: review?(ctalbert) → review+
Comment on attachment 529688 [details] [diff] [review]
ProcessOfflineManifest even when there's no manifest attribute

I landed the fix without the mochitest:
http://hg.mozilla.org/mozilla-central/rev/ecfea7698dee
At this point, it seems that it doesn't make sense to let a regression fix of a couple of lines to the parser get blocked by getting SpecialPowers in a shape that allows Fennec-compatible app cache testing.

Since this regression breaks a Web platform feature significantly compared to the Web platform feature working as specced in Firefox 3.6, nominating for Aurora.
Attachment #529688 - Flags: approval-mozilla-aurora?
(In reply to comment #24)
> 2. The failure mode here is pretty bad.  If this test fails, it will fail by
> timing out (never calling simpletest.finish()) and it will leave us in an
> offline state with an extra window running around, which will likely break
> most of the mochitests that follow it.

Addressed by adding a test-specific timer that fails and cleans up the test if the "cached" event doesn't fire within 2 seconds.

AFAICT, this is the best way to clean up the test on failure even though having test-specific timeouts sucks in principle.

> r+ with the changes

Thanks, but I was unable to come up with changes that would address your review comments.

> ::: parser/htmlparser/tests/mochitest/test_651441.html
> @@ +42,5 @@
> > +
> > +// Called from the onload handler in file_651441-1.html
> > +function startTest() {
> > +  SpecialPowers.pressNotificationButton(w, "offline-app-requested-mochi.test", 0);
> > +}
> 
> What's the failure mode here?  If something happens and the onload handler
> in file_651441-1.html gets processed before the inline script is activated
> then this could be called without a notification window present. We need to
> handle that error and fail the test (probably should call
> SimpleTest.finish() at that point after failing the test.

The onload handler is supposed always to run before the oncached handler. However, this point is now moot, because the test no longer tries to emulate a click to a particular piece of UI.

> @@ +59,5 @@
> > +}
> > +
> > +function navigateWindow() {
> > +  w.location = "file_651441-2.html";
> > +}
> 
> These three functions (goOffline, wipeCache, and navigateWindow) depend on
> being called sequentially but do not enforce the fact that they are called
> sequentially.  If they could detect that they are being called out of order,
> then they could fail the test and ensure we cleanup properly.

Done.

> @@ +67,5 @@
> > +  ok(true, "The script from app cache should have run.");
> > +  w.close();
> > +  SpecialPowers.setOfflineMode(false);
> > +  SimpleTest.finish();  
> > +}
> 
> The finish function is good.  But I'd factor out your finish and cleanup.  
> 
> function finish() {
>   ok (true, "script ran");
>   cleanup();
> }
> 
> function cleanup(){
>   w.close();
>   SpecialPowers.setOfflineMode(false);
>   SimpleTest.finish();
> }
> This way, if the click on the notification button fails you can still call
> cleanup and not hork the rest of the test run.

Done.

> ::: testing/mochitest/specialpowers/content/specialpowers.js
> @@ +208,5 @@
> > +      .getNotificationBox()
> > +      .getNotificationWithValue(notificationValue)
> > +      .childNodes[buttonIndex]
> > +      .click();
> > +  },
> 
> This needs to handle the case where the notification window doesn't exist
> and return some kind of indication that it failed.
> 
> Also, this code won't work in fennec, because fennec has no gBrowser.  So
> for fennec, you'll need to call getNotificationBox
> (http://mxr.mozilla.org/mozilla-central/ident?i=getNotificationBox) API,
> then you can navigate through the DOM to get the button you want to click.
> 
> You call the this API from the chrome window:
> window.getNotificationBox(aBrowser);

It turned out that window.getNotificationBox(aBrowser) was no good for Fennec, because _getTopChromeWindow is Elecrolysis-incompatible anyway. Yesterday, I discussed this with ted on IRC, and concluded that it's better to avoid the notification UI and to instead add API surface to SpecialPowers for adding and removing permissions. This is what attachment 532139 [details] [diff] [review] does.

However, this doesn't work with Fennec, either, because the content process can't add permissions! ted pointed out to me that I need to proxy the permission setting to the chrome process. I tried to do that with the changes in attachment 532140 [details] [diff] [review], but that breaks in both Firefox and Fennec. It's not clear to me why.

I think I need help with making permission setting from SpecialPowers work with both Firefox and Fennec. (As noted above, I went ahead and landed the HTML parser fix already, because it doesn't make sense to let it miss release trains while tweaking the test for Fennec compat.)
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Attachment #532140 - Flags: feedback?(ted.mielczarek)
Attachment #532140 - Flags: feedback?(ctalbert)
We need to land the patch in bug 642419, it makes hacking on content scripts really frustrating.
(In reply to comment #28)
> (In reply to comment #24)

> However, this doesn't work with Fennec, either, because the content process
> can't add permissions! ted pointed out to me that I need to proxy the
> permission setting to the chrome process. I tried to do that with the
> changes in attachment 532140 [details] [diff] [review] [review], but that breaks in both
> Firefox and Fennec. It's not clear to me why.
> 
> I think I need help with making permission setting from SpecialPowers work
> with both Firefox and Fennec. (As noted above, I went ahead and landed the
> HTML parser fix already, because it doesn't make sense to let it miss
> release trains while tweaking the test for Fennec compat.)
I can help with this.  And I agree with you on not holding the fix back from the train for this.  That makes sense to me too.  Sorry this turned into such a mess, I had no idea it was going to turn into such a bear.
Comment on attachment 529688 [details] [diff] [review]
ProcessOfflineManifest even when there's no manifest attribute

Renominating for beta, since this missed Firefox 5 in the aurora phase. Reason for nomination: This regression breaks a substantial aspect of the HTML5 application cache feature and is know to affect an actual 3rd-party product/service. Moreover, the fix is extremely simple: calling into the caching code with the empty string as the manifest URL when there's no manifest. We already had Firefox 3.6 do that, so the patch isn't doing anything more scary than what Firefox 3.6 did. (The spec requires the absence of the manifest attribute and the empty string as the value of the manifest attribute to be processed in the same way. Something that Firefox 4 and currently 5 fail to do.)

See comment 28 about the lack of a test case in this patch. The test case has succeeded locally and on try, so the code patch is known to work. Fennec/Electrolysis concerns are blocking the test case from actually landing.
Attachment #529688 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
It's possible (even probable?) that the permission problem you're having is caused by bug 621363. It's also a shame that my remoting work for permissions that I did last December never made it into the tree :<
(In reply to comment #33)
> It's possible (even probable?) that the permission problem you're having is
> caused by bug 621363.

That looks very probable. Thanks.
Attachment #529688 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 529688 [details] [diff] [review]
ProcessOfflineManifest even when there's no manifest attribute

Please land this change on both Aurora and Beta. (In the future, getting changes in during Aurora will save you this extra step.)
Attachment #529688 - Flags: approval-mozilla-aurora+
(In reply to comment #35)
> Please land this change on both Aurora and Beta.

Thanks for the approval. Landed:
http://hg.mozilla.org/releases/mozilla-aurora/rev/cb9883dbe8ba
http://hg.mozilla.org/releases/mozilla-beta/rev/ff660229576f

These landings have a number of burning tinderbox items associated with them. Those were caused by releng downtime and the builds have been retriggered.
Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

I want to verify this but I am not sure whether these are the correct steps:
 1. Load http://www.maquinasindustriales.org/bugzilla/page_a.html
 2. Allow the page to use local storage for the manifest
 3. Check the "Work offline" option from the File menu
 4. Go to http://www.maquinasindustriales.org/bugzilla/page_b.html
 5. As the page displays, the background should be yellow.

Can anyone look over them and tell me if I am right?

Thanks!
Hello George,

Yes, you are right but you should add a step between 3 and 4.
You must clean your cache (the default cache, not the "applicationCache").


(for everyone)
Please note that when FF 6.0 will be released, I will probably remove those pages from my website (maquinasindustriales). The files for the demo are still attached to this ticket (see "A Demo").
Comment on attachment 532140 [details] [diff] [review]
Diff to SpecialPowers that's supposed to make the test work with Electrolysis but breaks the test everywhere

Clearing old feedback request as this bug appears resolved and doesn't appear to need anything from me.  Sorry I missed this.
Attachment #532140 - Flags: feedback?(ctalbert)
Comment on attachment 532140 [details] [diff] [review]
Diff to SpecialPowers that's supposed to make the test work with Electrolysis but breaks the test everywhere

e10s is not a thing we care about at the moment, so we'll just let this slide.
Attachment #532140 - Flags: feedback?(ted.mielczarek)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: