Last Comment Bug 649778 - document.write may cause a document to be written to disk cache even when the page has Cache-Control: no-store
: document.write may cause a document to be written to disk cache even when the...
Status: RESOLVED FIXED
[sg:moderate]
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Michal Novotny (:michal)
:
Mentors:
Depends on: 748647
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-13 13:20 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2012-05-09 12:43 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (15.00 KB, patch)
2012-01-10 05:46 PST, Michal Novotny (:michal)
bzbarsky: review+
Details | Diff | Review
patch v2 - removed dump() from the test and made it a browser-chrome test (14.57 KB, patch)
2012-01-17 14:35 PST, Michal Novotny (:michal)
no flags Details | Diff | Review
patch v3 - fixed according to reviewer's comments (13.96 KB, patch)
2012-01-26 05:45 PST, Michal Novotny (:michal)
honzab.moz: review+
jaas: superreview+
Details | Diff | Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-04-13 13:20:28 PDT

    
Comment 1 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-12-15 05:03:41 PST
sg:moderate because, in theory, this could enable things like "Local storage of passwords in unencrypted form" from the rating guide.
Comment 2 Michal Novotny (:michal) 2012-01-10 05:46:13 PST
Created attachment 587281 [details] [diff] [review]
patch v1
Comment 3 Boris Zbarsky [:bz] 2012-01-10 17:55:22 PST
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.
Comment 4 Michal Novotny (:michal) 2012-01-17 14:35:23 PST
Created attachment 589303 [details] [diff] [review]
patch v2 - removed dump() from the test and made it a browser-chrome test
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-01-17 14:52:16 PST
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 6 Honza Bambas (:mayhemer) 2012-01-22 13:16:42 PST
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..
Comment 7 Michal Novotny (:michal) 2012-01-26 05:45:27 PST
Created attachment 591765 [details] [diff] [review]
patch v3 - fixed according to reviewer's comments

(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.
Comment 8 Honza Bambas (:mayhemer) 2012-01-26 11:18:05 PST
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.
Comment 9 Michal Novotny (:michal) 2012-01-26 11:29:33 PST
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 10 Honza Bambas (:mayhemer) 2012-01-27 06:05:01 PST
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.
Comment 12 Matt Brubeck (:mbrubeck) 2012-01-30 19:14:38 PST
https://hg.mozilla.org/mozilla-central/rev/4b7d5b27dd5f

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