Last Comment Bug 627047 - <html manifest="manifest"> in <pre> section of an HTML file pops offline application permission query up
: <html manifest="manifest"> in <pre> section of an HTML file pops offline appl...
Status: ASSIGNED
:
Product: Core
Classification: Components
Component: HTML: Parser (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Henri Sivonen (:hsivonen)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-19 09:01 PST by Honza Bambas (:mayhemer)
Modified: 2011-02-01 12:31 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case (114 bytes, text/html)
2011-01-19 09:01 PST, Honza Bambas (:mayhemer)
no flags Details
First attempt at a fix (3.35 KB, patch)
2011-01-31 08:12 PST, Henri Sivonen (:hsivonen)
no flags Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2011-01-19 09:01:10 PST
Not sure this is a valid bug, if this is not something very special in the spec, but the file I attach will popup the "This website (%S) is asking to store data on your computer for offline use" prompt, even it doesn't have a manifest defined on the root html element.

It happens because condition on line [1] is true (the manifest attribute is present in the array of attributes of the html root element).

[1] http://hg.mozilla.org/mozilla-central/annotate/809aded51aad/content/base/src/nsDocument.cpp#l4185
Comment 1 Honza Bambas (:mayhemer) 2011-01-19 09:01:58 PST
Created attachment 505082 [details]
Test case
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-01-19 09:52:15 PST
Yeah, this is the behavior the spec requires.  Attributes on later <body> and <html> are moved to the "canonical" <body> and <html>.  Websites actually depend on that.   :(
Comment 3 Honza Bambas (:mayhemer) 2011-01-19 10:06:00 PST
Ah, OK, then INVALIDATING this.  It just really surprised me...
Comment 4 Henri Sivonen (:hsivonen) 2011-01-20 02:38:28 PST
(In reply to comment #2)
> Yeah, this is the behavior the spec requires.  Attributes on later <body> and
> <html> are moved to the "canonical" <body> and <html>.

But that case isn't supposed to run the cache selection algorithm. Only the normal case runs it:
http://www.whatwg.org/specs/web-apps/current-work/multipage/tokenization.html#the-before-html-insertion-mode
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-01-20 06:49:48 PST
The cache selection kickoff doesn't happen in network....
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-01-20 06:53:44 PST
And in fact, it happens in the parser when it calls ProcessOfflineManifest, no?
Comment 7 Honza Bambas (:mayhemer) 2011-01-20 19:19:23 PST
It's scheduled in ProcessOfflineManifest to happen after document load (on STATE_STOP state flag).


And I can confirm an update is triggered by the test case.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-01-20 19:40:43 PST
If it happens after document load, then the DOM code certainly has no idea where the attr value came from... (not that it ever does, really).  Honza, does ProcessOfflineManifest happen early enough?  Should we be caching the then-current value of the attribute when it happens?
Comment 9 Henri Sivonen (:hsivonen) 2011-01-21 04:28:35 PST
(In reply to comment #8)
> If it happens after document load, then the DOM code certainly has no idea
> where the attr value came from...

The DOM code should pay no attention to the attribute:
http://www.whatwg.org/specs/web-apps/current-work/#attr-html-manifest

The only way to register a manifest should be via nsContentSink as called by the parser.
Comment 10 Honza Bambas (:mayhemer) 2011-01-23 12:29:08 PST
(In reply to comment #9)
> The DOM code should pay no attention to the attribute:
> http://www.whatwg.org/specs/web-apps/current-work/#attr-html-manifest

Not sure what you mean.  The spec says the attribute cannot be later changed by js code (dynamically).  But in this case the modification comes from the parser.

(In reply to comment #8)
> Honza, does
> ProcessOfflineManifest happen early enough?  

It is, AFAIK, sequentially processed with other html/dom nodes and attributes coming.  So we could say it is early enough.  But, ProcessOfflineManifest is not called in case there is no manifest attrib at all.

> Should we be caching the
> then-current value of the attribute when it happens?

I'd say we should "close" modifications to this attribute after we process the first occurrence of the <html> opening tag.  This may happen in the parser or content sink, if it has knowledge of finishing <html> opening tag processing.


But, as noted above, the spec allows more then one occurrence of the <html> tag.  So the question is, if we should rather allow the manifest attribute processing on the second (third, etc.) occurrence of the <html> tag.  This is not against the offline application selection algorithm spec, at least I don't about anything we break with allowing it.  We are only missing a protection against multiple occurrences of the manifest attribute.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-01-25 20:42:57 PST
> But in this case the modification comes from the parser.

How do we tell the two apart?  And why?

> This may happen in the parser or content sink, if it has knowledge of finishing
> <html> opening tag processing.

It does, sure....

> But, as noted above, the spec allows more then one occurrence

See comment 4.

It really sounds like the cache selection code is not running at the time the spec says to run it, which is why we run into this problem.
Comment 12 Honza Bambas (:mayhemer) 2011-01-26 14:44:54 PST
Now I understand.  We call the cache selection algorithm also for other occurrences of opening html tag.  Does HTMLContentSink::OpenContainer know we are in state other then "before html"?  If so, we can just bypass call to ProcessOfflineManifest() that is actually the cache selection algorithm implementation.

When html opening tag is found during state "in body", the attributes have to be merged, we do that correctly.  However, that leads to the prompt for offline apps privileges.
Comment 13 Honza Bambas (:mayhemer) 2011-01-26 14:48:23 PST
It means: I would expect the prompt will raise only when there is a valid manifest attribute value on the first occurrence of the html tag.  Other occurrences, even empty, should not be considering to pop the prompt up.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-01-26 18:12:30 PST
With the HTML5 parser we shouldn't be ending up in HTMLContentSink::OpenContainer....
Comment 15 Henri Sivonen (:hsivonen) 2011-01-31 00:24:32 PST
So per spec:
 1) The content tree should never trigger the cache selection algorithm.
 2) The parser should trigger the cache selection algorithm but only when it is actually creating the root element (*not* when it sees an <html> token later).
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-01-31 05:13:14 PST
Henri, did you mean to change the component and target milestone?
Comment 17 Henri Sivonen (:hsivonen) 2011-01-31 06:04:04 PST
(In reply to comment #16)
> Henri, did you mean to change the component and target milestone?

No. And this isn't the first time Minefield has spontaneously changed Bugzilla components for me. I wonder what exactly makes this happen.
Comment 18 Henri Sivonen (:hsivonen) 2011-01-31 07:48:18 PST
(In reply to comment #0)
> It happens because condition on line [1] is true (the manifest attribute is
> present in the array of attributes of the html root element).
> 
> [1]
> http://hg.mozilla.org/mozilla-central/annotate/809aded51aad/content/base/src/nsDocument.cpp#l4185

That's where the bug is. That code shouldn't be reading the attributes. Instead, nsContentSink::ProcessOfflineManifest should set a flag on nsIDocument and then that code should check the flag.
Comment 19 Henri Sivonen (:hsivonen) 2011-01-31 08:12:58 PST
Created attachment 508408 [details] [diff] [review]
First attempt at a fix

Hasn't been through the tryserver yet.
Comment 20 Honza Bambas (:mayhemer) 2011-01-31 10:05:15 PST
Comment on attachment 508408 [details] [diff] [review]
First attempt at a fix

This is fix for one part of the problem.  We also have to bypass nsContentSink::ProcessOfflineManifest when this new flag has been set (to not trigger the cache selection algorithm again).  Also please don't forget to initialize it in all constructors of nsContentSink.
Comment 21 Henri Sivonen (:hsivonen) 2011-02-01 03:09:49 PST
(In reply to comment #20)
> Comment on attachment 508408 [details] [diff] [review]
> First attempt at a fix
> 
> This is fix for one part of the problem.  We also have to bypass
> nsContentSink::ProcessOfflineManifest when this new flag has been set (to not
> trigger the cache selection algorithm again).

I believe the HTML parser calls nsContentSink::ProcessOfflineManifest at most once. (Might be worth adding an assertion to that effect.)

I didn't check what the XML content sink does. I'll check.

> Also please don't forget to
> initialize it in all constructors of nsContentSink.

Did you mean the constructor of nsIDocument? I didn't add anything to the constructor there, because the class seemed to have zeroing operator new.
Comment 22 Henri Sivonen (:hsivonen) 2011-02-01 08:47:02 PST
(In reply to comment #21)
> I didn't check what the XML content sink does. I'll check.

Looks OK.

However, on tryserver, these two fail:
1095 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_offlineNotification.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - notification is null at http://mochi.test:8888/tests/browser/base/content/test/test_offlineNotification.html:108
1098 ERROR TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_offline_gzip.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - notification is null at http://mochi.test:8888/tests/browser/base/content/test/test_offline_gzip.html:107

Not sure yet why the tests don't see the notification bar.
Comment 23 Honza Bambas (:mayhemer) 2011-02-01 12:08:20 PST
(In reply to comment #22)
> Not sure yet why the tests don't see the notification bar.

I've seen this also when I broke scheduleUpdate of nsOfflineCacheUpdateService.  But your case might be different.



(In reply to comment #21)
> I believe the HTML parser calls nsContentSink::ProcessOfflineManifest at most
> once. (Might be worth adding an assertion to that effect.)

I was testing that and I think it's called more then ones with the test case.  Worth to add an assertion.  I'll try this ones again my self anyway.
Comment 24 Honza Bambas (:mayhemer) 2011-02-01 12:31:08 PST
(In reply to comment #23)
> I was testing that and I think it's called more then ones with the test case. 
> Worth to add an assertion.  I'll try this ones again my self anyway.

Ok, not with the attached test case, but with a test case modified like this:

<!DOCTYPE html>
<html manifest="manifest.cacheManifest">
  <body>
    <html manifest="manifest.cacheManifest">
..

I get three (3) calls to nsContentSink::ProcessOfflineManifest, all of them invoke an update.

Note You need to log in before you can comment on or make changes to this bug.