Closed Bug 543726 Opened 10 years ago Closed 10 years ago

[HTML5] dom/tests/mochitest/ajax/offline/test_foreign.html fails

Categories

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

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: hsivonen, Assigned: mayhemer)

Details

Attachments

(1 file, 1 obsolete file)

dom/tests/mochitest/ajax/offline/test_foreign.html with html5.enable. Not known why, yet.
dom/tests/mochitest/ajax/offline/test_updatingManifest.html times out.
In test_foreign.html, creating the cache entry fails with NS_ERROR_NOT_AVAILABLE when updating the manifest when the page has already been loaded from the app cache once.
When the manifest is being updated, mCacheEntry on nsHttpChannel is null, which causes the above-mentioned NS_ERROR_NOT_AVAILABLE.

Honza, how is mCacheEntry supposed to become initialized by the time the manifest is reprocessed?
Looks like you are doing nsHtml5SpeculativeLoader::ProcessManifest too late.  nsHttpChannel releases mCacheEntry on several occasions, one of them is nsHttpChannel::OnStopRequest, so, right after the page load finished.

I believe we do not need the cache entry in the failing code, we just need the cache entry key that can be held in the channel and obtained through a new getter.

I'm very soon about to do exactly this change for e10s code (to get rid of nsICacheEntryDescriptor usage on the content process).  So, we can change it right now on m-c, I'll have a new patch soon.
Attached patch v1 (obsolete) — Splinter Review
This patch is changing nsICachingChannel interface.  HttpChannel now holds the cache entry key generated during OpenCacheEntry and provides it even after the cache entry has been closed.

This is needed for asynchronous processing of the manifest attribute (html5 parser) and this is also a pre-solution for e10s code.  e10s patch will be very similar.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #429337 - Flags: review?(jduell.mcbugs)
test_updatingManifest.html works for me (with this patch).
(In reply to comment #4)
> Looks like you are doing nsHtml5SpeculativeLoader::ProcessManifest too late. 
> nsHttpChannel releases mCacheEntry on several occasions, one of them is
> nsHttpChannel::OnStopRequest, so, right after the page load finished.

I see. Since the HTML5 parser just ships the stream data to the parser thread, it's not guaranteed that anything has been actually processed by the time the channel fires its OnStopRequest on the main thread.

> So, we can change it
> right now on m-c, I'll have a new patch soon.

Great! Thank you!
Attachment #429337 - Flags: superreview?(cbiesinger)
Attachment #429337 - Flags: review?(jduell.mcbugs)
Attachment #429337 - Flags: review+
Comment on attachment 429337 [details] [diff] [review]
v1

>diff --git a/netwerk/base/public/nsICachingChannel.idl b/netwerk/base/public/nsICachingChannel.idl
>+    /**
>+     * The full cache entry key, the string used to find a cache entry
>+     * in a cache session.  This is the key generated during time the channel
>+     * opens the cache entry.
>+     */
>+    readonly attribute ACString cacheEntryKey;

Description needs editing.  How about

>+     * The cache entry key: the string used to find a cache entry
>+     * in a cache session.  Generated when the channel opens the cache entry.

>diff --git a/netwerk/protocol/http/src/nsHttpChannel.cpp b/netwerk/protocol/http/src/nsHttpChannel.cpp
> 
> NS_IMETHODIMP
>+nsHttpChannel::GetCacheEntryKey(nsACString &cacheKey)
>+{
>+    cacheKey = mCacheEntryKey;
>+    return NS_OK;
>+}

Would it be worth returning NS_NOT_AVAILABLE if mCacheEntryKey hasn't been set yet?  


>diff --git a/netwerk/protocol/http/src/nsHttpChannel.h b/netwerk/protocol/http/src/nsHttpChannel.h
>--- a/netwerk/protocol/http/src/nsHttpChannel.h
>+++ b/netwerk/protocol/http/src/nsHttpChannel.h
> 
>+    // Cached cache entry key to access it after the cache entry has been 
>+    // closed
>+    nsCString                         mCacheEntryKey;

Slight documentation edit: how about

>+    // Cache entry key: may be accessed after the cache entry has been 
>+    // closed

Otherwise looks fine.  This is an IDL change, so needs +sr.  bz is out this week, so trying biesi.
Attachment #429337 - Flags: superreview?(cbiesinger) → superreview?(bzbarsky)
Question.  Is there a reason we can't make GetCacheKey/SetCacheKey take the whole key instead of just the post id part?  Would that be more of an API change than adding this new method?
(In reply to comment #9)
> Question.  Is there a reason we can't make GetCacheKey/SetCacheKey take the
> whole key instead of just the post id part?  Would that be more of an API
> change than adding this new method?

DocShell is moving cacheKey here and there between shentries and channels (line 7947, line 8352, line 9009, line 9456).  It is being set on the channel in nsDocShell::DoURILoad.  I am not familiar with post ids but I presume only the post id should be changed by call of cacheChannel->SetCacheKey().

This also affects sessionStore.js.
Docshell should only be moving the cache key between channels/shentries that have identical URIs, I would think... though maybe not modulo ref.  But the ref is not included in the cache key anyway, right?
Refs are removed from the cache key, right.

If it's that way, then we might change the API.  Are there tests that may reveal any bugs involved by this change?  Isn't this risky in any way from a security point of view?
> If it's that way, then we might change the API.

Right, hence the question in comment 9.

> Are there tests that may reveal any bugs involved by this change? 

Unlikely.  :(

> Isn't this risky in any way from a security point of view?

I don't think it is, but who knows what extensions do with this stuff.  :(
We can get the whole cache key w/o API modification but we can only set the post ID.

After I finished writing the new class I realized it would be better to have a new interface rather, like nsIHttpCacheKey : nsISupportsPRUint32 { r/o string cacheKey };

It is IMHO not a good idea to allow set of the whole cacheKey (use nsISupportsCString instead) because it is generated when needed.  It would complicate the code and I still don't believe it would be safe.  However, the IDL comment on the cacheKey attribute says it works with the whole cache key allowing to be set, only constrained by the same URI.
Attachment #429337 - Attachment is obsolete: true
Attachment #434667 - Flags: review?(bzbarsky)
Attachment #429337 - Flags: superreview?(bzbarsky)
bz, got an ETA for doing this review? This is getting close to being the only remaining blocker for turning on the HTML5 parser by default...
Comment on attachment 434667 [details] [diff] [review]
v2 [Checkin comment 17]

r=bzbarsky
Attachment #434667 - Flags: review?(bzbarsky) → review+
Comment on attachment 434667 [details] [diff] [review]
v2 [Checkin comment 17]

http://hg.mozilla.org/mozilla-central/rev/64250317a586
Attachment #434667 - Attachment description: v2 → v2 [Checkin comment 17]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.