Open Bug 627047 Opened 13 years ago Updated 2 years ago

<html manifest="manifest"> in <pre> section of an HTML file pops offline application permission query up

Categories

(Core :: DOM: HTML Parser, defect, P5)

Product:
Core
Component:
DOM: HTML Parser
Type:
defect

Tracking

()

People

(Reporter: mayhemer, Unassigned)

Details

Attachments

(2 files)

Test case
114 bytes, text/html
Details
First attempt at a fix
3.35 KB, patch
Details | Diff | Splinter Review 
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
Attached file Test case — 

    
    

    
  
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.   :(

    
    

    
  
Ah, OK, then INVALIDATING this.  It just really surprised me...
Status: UNCONFIRMED → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID

    
    

    
  
(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
Status: RESOLVED → REOPENED
Component: HTML: Parser → Networking: Cache
Ever confirmed: true
QA Contact: parser → networking.cache
Resolution: INVALID → ---

    
    

    
  
The cache selection kickoff doesn't happen in network....
Component: Networking: Cache → DOM
QA Contact: networking.cache → general

    
    

    
  
And in fact, it happens in the parser when it calls ProcessOfflineManifest, no?
Component: DOM → HTML: Parser
QA Contact: general → parser

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
> 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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
With the HTML5 parser we shouldn't be ending up in HTMLContentSink::OpenContainer....

    
    

    
  
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).
Component: HTML: Parser → Image Blocking
Target Milestone: --- → mozilla2.0b10

    
    

    
  
Henri, did you mean to change the component and target milestone?

    
    

    
  
(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.
Component: Image Blocking → HTML: Parser
Target Milestone: mozilla2.0b10 → ---

    
    

    
  
(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.
Assignee: nobody → hsivonen
Status: REOPENED → ASSIGNED

    
    

    
  


  
Hasn't been through the tryserver yet.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
Leaving this open until AppCache is removed but not actively working on this.
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
Priority: -- → P5

    
  
Severity: normal → S3

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: