938186 - HTTP cache v2: introduce DISALLOW_SYNC_CALLBACK for opening cache entries

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

After some review discussion with Michal, it seems like a good idea to prevent the unexpected synchronous call of the open callback when using the new cache backend (i.e. onCacheEntryCheck and onCacheEntryAvailable can be called sooner then respective asyncOpenURI exits).

I can imagine extension developers will be introducing hacks like isInAsyncOpenURI flags etc.

Wisdom is at the place here.

Deps on bug 922741, which introduces a base code to easily play with the callback management and threading.

Comment 1

[Honza Bambas (:mayhemer)]

[ No longer blocks enabling cache2, expected to be fixed after cache2 is on ]

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

- looks big, but is actually quite simple
- two tests to 1) check the existing test (01-basic) scenario still works and 2) that the sync nature is not broken
- not changing the iid since there is no need when only adding a flag

Comment 3

[Michal Novotny [:michal]]

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

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

::: netwerk/cache2/nsICacheStorage.idl
@@ +50,5 @@
>     */
>    const uint32_t CHECK_MULTITHREADED = 1 << 4;
>  
>    /**
> +   * Forces the callback to invoke always only asynchronously regardless we have

to be invoked

@@ +51,5 @@
>    const uint32_t CHECK_MULTITHREADED = 1 << 4;
>  
>    /**
> +   * Forces the callback to invoke always only asynchronously regardless we have
> +   * all the information to invoked directly from inside asyncOpenURI method.

to invoke it directly

Comment 4

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/940d041073c7

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0dbdb35eace

https://tbpl.mozilla.org/php/getParsedLog.php?id=41129773&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41128719&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41129570&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=41129856&tree=Mozilla-Inbound

Comment 6

[Honza Bambas (:mayhemer)]

Theory:
- the writer (the first callbacks) is released by gc between return from most nested asyncOpenCacheEntry callback function and assignment outOfAsyncOpen3 = true;
- this means that we invoke the callback intended to be reposted during call of OnHandleClosed with aHandle == mWriter which synchronously calls InvokeCallbacks
- the dispatch of InvokeCallbacksLock is then just a no-op

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v2

- introduced mDispatchingCallbacks flag set between redispatch of InvokeCallbacks and actual fire on the target thread
- this way any sudden call to InvokeCallback(s) does nothing so that the condition of asynchronicity is 100% fulfilled
- not using the existing mPreventCallbacks flag since that is used for a different purpose and could well collide with the dispatch-wait purpose
- force async check removed from InvokeAvailableCallback (actually from OnAvailThread) since it's not needed, each callback must pass the InvokeCallbacks/InvokeCallback path
- patch is actually simpler then the one before

https://tbpl.mozilla.org/?tree=Try&rev=4b718272e9c6

Comment 8

[Honza Bambas (:mayhemer)]

retry on a greened push:
https://tbpl.mozilla.org/?tree=Try&rev=8d22bc9bc311

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #8)
> retry on a greened push:
> https://tbpl.mozilla.org/?tree=Try&rev=8d22bc9bc311

Grrr... bug 1005696!

Landing now.

Comment 10

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=400d92d915f4

Comment 11

[Honza Bambas (:mayhemer)]

Backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/08974ed00874

The flag here is not of help (but it should be there anyway), I believe this a thread safety of the flags.  I also tried to push with mForceAsync being a normal bool (not a bit) and no orange...  So, the flags access needs careful check probably.  There is some thread race condition!

Comment 12

[Honza Bambas (:mayhemer)]

OK, checked for thread races, all the time we access flags for write (and read of the new flag) under the lock.  I'm getting out of ideas here, will try to get logging from try, however I cannot reproduce on try with an exactly the same patch as is on m-i...  Really strange.

Also, since there is no big pressure on fixing add-ons (there is none that needs to migrate to cache1 actually!) I vote for postponing this bug or even WONTFIXING it.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #12)
> to cache1 

cache2 of course...

Comment 14

[Honza Bambas (:mayhemer)]

Closing, since no one complains - the patch is actually not needed.

Comment 15

[Josh Matthews [:jdm]]

Attachment: "HTTP cache v2: introduce DISALLOW_SYNC_CALLBACK for opening cache entries" [

Here's a rebased version.

Comment 16

[Josh Matthews [:jdm]]

https://tbpl.mozilla.org/?tree=Try&rev=63af5aca3f6f