Closed Bug 649778 Opened 13 years ago Closed 12 years ago

document.write may cause a document to be written to disk cache even when the page has Cache-Control: no-store

Categories

(Core :: Networking: Cache, defect)

Product:
Core
Component:
Networking: Cache
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla12

People

(Reporter: briansmith, Assigned: michal)

References

Details

(Whiteboard: [sg:moderate])

Attachments

(1 file, 2 obsolete files)

patch v3 - fixed according to reviewer's comments
13.96 KB, patch
mayhemer
: review+
jaas
: superreview+
Details | Diff | Splinter Review 
      No description provided.
sg:moderate because, in theory, this could enable things like "Local storage of passwords in unencrypted form" from the rating guide.
Whiteboard: [sg:moderate]
Attached patch patch v1 (obsolete) — — Splinter Review 

        Attachment #587281 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 587281 [details] [diff] [review]
patch v1

Please take the random dump() calls out of the test?

Also, please don't add UniversalXPConnect goop to the tests.  If you need more APIs exposed on SpecialPowers, do that.  Or make this a browser-chrome test, if that's simpler.

The HTTP parts could use review from someone familiar with that code better (maybe Honza?).

r=me modulo those issues.

        Attachment #587281 -
        Flags: review?(honzab.moz)

        Attachment #587281 -
        Flags: review?(bzbarsky)

        Attachment #587281 -
        Flags: review+

    
    

    
  


  

        Attachment #587281 -
        Attachment is obsolete: true

        Attachment #587281 -
        Flags: review?(honzab.moz)

        Attachment #589303 -
        Flags: review?(honzab.moz)

    
    

    
  
My understanding is that we do this in order to support session restore. What is the effect that this has on session restore? We should evaluate how this would effect GMail, buzilla, HotMail, Yahoo! Mail, etc. regarding session restore?

We may need to document this change in the release notes and/or on MDN, if it has a negative impact on session restore.

    
    

    
  
Comment on attachment 589303 [details] [diff] [review]
patch v2 - removed dump() from the test and made it a browser-chrome test

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

Dropping the r flag for now, see the comments please.

::: content/html/content/test/browser_bug649778.js
@@ +1,1 @@
> +// Test for bug 649778 - #649778 - document.write may cause a document to be written to disk cache even when the page has Cache-Control: no-store

Duplicated bug #.

::: content/html/document/src/nsHTMLDocument.cpp
@@ +1635,5 @@
> +
> +    loadFlags |= nsIRequest::INHIBIT_PERSISTENT_CACHING;
> +    rv = channel->SetLoadFlags(loadFlags);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

Just a tip: as I sense we might want to carry more flags in the future, it might be a bit more elegant to do directly channel.loadFlags |= callerChannel.loadFlags & INHIBIT_PERSISTENT_CACHING, i.e. not just carry the the one flag through inhibitPersistentCaching local bool but bitwise-or with masked caller flags.

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +728,5 @@
> +  nsCAutoString tmpStr;
> +  rv = mCacheEntry->GetMetaDataElement("inhibit-persistent-caching",
> +                                       getter_Copies(tmpStr));
> +  if (tmpStr == NS_LITERAL_CSTRING("1"))
> +    mLoadFlags |= INHIBIT_PERSISTENT_CACHING;

No check on |rv| ?

However, I don't understand what this change is needed for.  This piece of code is not covered by the test and I also think this is wrong.  INHIBIT_PERSISTENT_CACHING is tested in OpenCacheEntry, ReadFromCache is obviously called later.

Or is this here probably because of the code in nsHTMLDocument?

But this gets me to another issue with this patch: 
- setup the profile to remember the last windows
- load a file with no-store and the same content as the testing file
- close firefox
- start it again
=> I can see wyciwyg://0/http://example.com/path/to/file in the address bar and content of the page is empty

I checked that this doesn't happen w/o the patch and seems not to be any race condition (tried 3 times w/ and w/o the patch, result is consistent).  

Apparently we are breaking session store with this, when the file and the original URL is not found in the cache.

Not sure how to proceed here.  The patch it self prevents caching, even when the written document writes one more time: 

document.write('<html><body onload="setTimeout(function() {document.open(); document.write(\'<html>WYCIWYG DOCUMENT 2</html>\'); document.close();}, 2000)">WYCIWYG DOCUMENT</body></html>');

Not sure what the fix for session store should be here atm..

        Attachment #589303 -
        Flags: review?(honzab.moz)

    
    

    
  


  
(In reply to Honza Bambas (:mayhemer) from comment #6)
> But this gets me to another issue with this patch: 
> - setup the profile to remember the last windows
> - load a file with no-store and the same content as the testing file
> - close firefox
> - start it again
> => I can see wyciwyg://0/http://example.com/path/to/file in the address bar
> and content of the page is empty
> 
> I checked that this doesn't happen w/o the patch and seems not to be any
> race condition (tried 3 times w/ and w/o the patch, result is consistent).  
> 
> Apparently we are breaking session store with this, when the file and the
> original URL is not found in the cache.

That's exactly a goal of this bug/patch. Imagine following situation:

    nostore_doc -> wyciwyg_doc1 -> wyciwyg_doc2

This means: some no store document generates another document which generates yet another document. As of now we don't store in the disk cache only nostore_doc, which is considered as a wrong behavior. 


> ::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
> @@ +728,5 @@
> > +  nsCAutoString tmpStr;
> > +  rv = mCacheEntry->GetMetaDataElement("inhibit-persistent-caching",
> > +                                       getter_Copies(tmpStr));
> > +  if (tmpStr == NS_LITERAL_CSTRING("1"))
> > +    mLoadFlags |= INHIBIT_PERSISTENT_CACHING;
> 
> No check on |rv| ?
> 
> However, I don't understand what this change is needed for.  This piece of
> fcode is not covered by the test and I also think this is wrong. 
> INHIBIT_PERSISTENT_CACHING is tested in OpenCacheEntry, ReadFromCache is
> obviously called later.
> 
> Or is this here probably because of the code in nsHTMLDocument?

Yes, this code sets INHIBIT_PERSISTENT_CACHING flag so that nsHTMLDocument can read it. This is needed in case you read the wycywig document from the cache. E.g. for this scenario:

1) nostore doc generates wyciwyg1 doc
2) visit http://www.foo.com by typing URL into URL bar
3) go back in history, the wyciwyg1 doc is read from the cache
4) wyciwyg1 doc generates wyciwyg2 doc

Wycywyg2 would be stored in the disk cache without setting the flag in step 3.

        Attachment #589303 -
        Attachment is obsolete: true

        Attachment #591765 -
        Flags: review?(honzab.moz)

    
    

    
  
Michal, if this is about to get to next Aurora, please find a different reviewer, maybe Boris.  I won't be able to do this right now.

    
    

    
  
It actually already has r+ from Boris. He just wanted someone else to look into the changes in HTTP protocol. Could you review this part?

    
    

    
  
Comment on attachment 591765 [details] [diff] [review]
patch v3 - fixed according to reviewer's comments

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

r=honzab for the net/http parts.

        Attachment #591765 -
        Flags: review?(honzab.moz) → review+

    
  

        Attachment #591765 -
        Flags: superreview?(joshmoz)

    
  

        Attachment #591765 -
        Flags: superreview?(joshmoz) → superreview+

    
    

    
  
http://hg.mozilla.org/integration/mozilla-inbound/rev/4b7d5b27dd5f
Whiteboard: [sg:moderate] → [sg:moderate] [inbound]
Target Milestone: --- → mozilla12

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/4b7d5b27dd5f
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [sg:moderate] [inbound] → [sg:moderate]
Depends on: 748647

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: