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

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

dom/tests/mochitest/ajax/offline/test_foreign.html with html5.enable. Not known why, yet.

Comment 1

[Henri Sivonen (:hsivonen)]

dom/tests/mochitest/ajax/offline/test_updatingManifest.html times out.

Comment 2

[Henri Sivonen (:hsivonen)]

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.

Comment 3

[Henri Sivonen (:hsivonen)]

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?

Comment 4

[Honza Bambas (:mayhemer)]

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.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1

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.

Comment 6

[Honza Bambas (:mayhemer)]

test_updatingManifest.html works for me (with this patch).

Comment 7

[Henri Sivonen (:hsivonen)]

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

Comment 8

[Jason Duell]

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.

Comment 9

[Boris Zbarsky [: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?

Comment 10

[Honza Bambas (:mayhemer)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Honza Bambas (:mayhemer)]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2 [Checkin comment 17]

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.

Comment 15

[Johnny Stenback (:jst)]

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 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 434667 [details] [diff] [review]
v2 [Checkin comment 17]

r=bzbarsky

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 434667 [details] [diff] [review]
v2 [Checkin comment 17]

http://hg.mozilla.org/mozilla-central/rev/64250317a586