917423 - HTTP cache v2: Migrate Wyciwyg to the new cache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

It doesn't necessarily block bug 913806 but should be fixed by that time.

Wyciwyg is using own cache session names, but the new cache is distinguishing by the full URL so the new API can be well and easily used.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: WIP1

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

- wyciwyg channel logic left the same, only uses the new API
- using AsyncOpenURI in place of OpenCacheEntry when creating new entry (OPEN_TRUNCATE a.k.a ACCESS_WRITE), gives the result (the new entry) via the callback synchronously
- nsWyciwygChannel::WriteToCacheEntryInternal makes sure the entry we already have is recreated for write/only
- only one test needed to be updated to the new cache API

The linux32-only failures were caused by a stupid mistake with uninitialized |rv| local var that none of the compilers had caught.


Here are complete try runs of all blocking bugs stack:
https://tbpl.mozilla.org/?tree=Try&rev=1b6836d6a4d1 (cache1)
https://tbpl.mozilla.org/?tree=Try&rev=780e4c80e223 (cache2)

Comment 3

[Honza Bambas (:mayhemer)]

Still some intermittent timeouts seen on gum with these patches and cache2 on:

https://tbpl.mozilla.org/php/getParsedLog.php?id=30352757&tree=Gum
https://tbpl.mozilla.org/php/getParsedLog.php?id=30353004&tree=Gum

Please review for cache2=off anyway, cache2=on issues can be fixed in followups.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> Still some intermittent timeouts seen on gum with these patches and cache2
> on:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30352757&tree=Gum
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30353004&tree=Gum

I think that push https://hg.mozilla.org/projects/gum/rev/7776d94dbafe (related to bug 934616) may fix them.

Watch for https://tbpl.mozilla.org/?tree=Gum&rev=7776d94dbafe

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 827988 [details] [diff] [review]
v1

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

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +394,5 @@
>    // open a cache entry for this channel...
> +  // mIsPending set to true since OnCacheEntryAvailable may be called
> +  // synchronously and fails when mIsPending found false.
> +  mIsPending = true;
> +  nsresult rv = OpenCacheEntry(mURI, nsICacheStorage::OPEN_NORMALLY |

Use OPEN_READONLY since we don't want to create entry when using nsWyciwygChannel for reading.

@@ +430,5 @@
>    NS_ASSERTION(IsOnCacheIOThread(), "wrong thread");
>  
>    nsresult rv;
>  
> +  if (mCacheEntry && !mCacheEntryIsWriteOnly) {

This can never happen. nsWyciwygChannel is used either only for reading (via AsyncOpen) or only for writing (via Open).

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> > Still some intermittent timeouts seen on gum with these patches and cache2
> > on:
> > 
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=30352757&tree=Gum
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=30353004&tree=Gum
> 
> I think that push https://hg.mozilla.org/projects/gum/rev/7776d94dbafe
> (related to bug 934616) may fix them.
> 
> Watch for https://tbpl.mozilla.org/?tree=Gum&rev=7776d94dbafe

Didn't help (as I thought actually), TEST-UNEXPECTED-FAIL | /tests/parser/htmlparser/tests/mochitest/test_bug715739.html | Test timed out on Rev5 MacOSX Mountain Lion 10.8 gum opt test mochitest-5

Hope this is the problem with READONLY opening...

Comment 7

[Honza Bambas (:mayhemer)]

New patches pushed:
https://tbpl.mozilla.org/?tree=Gum&rev=41ba8a1e752f (cache2)
https://tbpl.mozilla.org/?tree=Try&rev=44eed13d74fe (cache1)

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> New patches pushed:
> https://tbpl.mozilla.org/?tree=Gum&rev=41ba8a1e752f (cache2)
> https://tbpl.mozilla.org/?tree=Try&rev=44eed13d74fe (cache1)

Still having timeout problems on gum (with cache2 on):
https://tbpl.mozilla.org/php/getParsedLog.php?id=30544720&tree=Gum
https://tbpl.mozilla.org/php/getParsedLog.php?id=30565662&tree=Gum
https://tbpl.mozilla.org/php/getParsedLog.php?id=30544329&tree=Gum
https://tbpl.mozilla.org/php/getParsedLog.php?id=30565516&tree=Gum

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2

This patch works well with cache2=off what is OK for now to go forward.  There are though intermittent timeouts of two tests, mainly on osx opt builds.  It's hard to reproduce locally and hard to track directly on the try server.

I will solve them separately in followup bugs blocking cache2=on.

See interdiff (coming) for actual changes v1 -> v2.

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1->v2 interdiff

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 833343 [details] [diff] [review]
v2

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

(In reply to Michal Novotny (:michal) from comment #5)
> > +  if (mCacheEntry && !mCacheEntryIsWriteOnly) {
> 
> This can never happen. nsWyciwygChannel is used either only for reading (via
> AsyncOpen) or only for writing (via Open).

In fact, writing to the entry is initialized via WriteToCacheEntry and not via Open. Anyway, wyciwyg channel is written so that it can either write to the entry, or read from the entry, i.e. WriteToCacheEntry must not be called once AsyncOpen was called and vice versa. So the member mCacheEntryIsWriteOnly is not needed. To be sure that the channel is not used in a wrong way, the best would be IMO to introduce some mMode enum with values NONE, WRITING, READING. Then put MOZ_ASSERT(mMode == NONE) into AsyncOpen and MOZ_ASSERT(mMode != READING) into WriteToCacheEntry.

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +415,4 @@
>      return rv;
>    }
>  
> +  // There is no code path that would invoke the listener sooner then

then -> than

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v2.1

Mostly as suggested (v2->v2.1):

- removed mCacheEntryWriteOnly flag
- added mState { NONE, READING, WRITING }
- initially NONE
- AsyncOpen checks it's NONE and fails if not
- AsyncOpen sets to READING
- WriteToCacheEntry check it's not READING and fails if it is
- WriteToCacheEntry sets to WRITING

https://tbpl.mozilla.org/?tree=Try&rev=1bbb698e4b7a

Comment 13

[Michal Novotny [:michal]]

Comment on attachment 8348797 [details] [diff] [review]
v2.1

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

r+ with following nits fixed

::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +400,5 @@
>  {
>    LOG(("nsWyciwygChannel::AsyncOpen [this=%p]\n", this));
>  
>    NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);
> +  NS_ENSURE_TRUE(mMode == NONE, NS_ERROR_IN_PROGRESS);

WriteToCacheEntry() asserts when the mode is wrong, but AsyncOpen() just fails. Please add the assertion to have the same behavior.

@@ +418,4 @@
>      return rv;
>    }
>  
> +  // There is no code path that would invoke the listener sooner then

You didn't fix the typo.

Comment 14

[Honza Bambas (:mayhemer)]

Attachment: v2.2 [ready to land]

Thanks, updated.

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8349452 [details] [diff] [review]
v2.2 [ready to land]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a081f99b568d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a081f99b568d