1554652 - Implement asynchronous opening of alternative output stream

Status: NEW

(Core :: Networking: Cache, enhancement, P5)

Description

[Michal Novotny [:michal]]

nsICacheEntry::openAlternativeOutputStream fails if there is any input stream opened for already existing alternative data. Asynchronous version should wait until last input stream is closed and then provide output stream via a callback.

Comment 1

[Luke Wagner [:luke]]

Honza: a few weeks ago you said to ni? you to get your attention. For context: this patch blocks bug 1487113, and thus all of wasm alt-data caching. Thanks!

Comment 2

[Honza Bambas (:mayhemer)]

I don't remember what exactly you need from me on this one, assigned to Michal.

Comment 3

[Luke Wagner [:luke]]

Really just knowing how or who to get this bug fixed, so as to unblock bug 1487113.

Comment 4

[Honza Bambas (:mayhemer)]

Ah! I recall now :)) Yes, changing to P1 so we try to find an assignee soon, for this release.

Comment 5

[Luke Wagner [:luke]]

Any luck on finding an assignee?

Comment 6

[Honza Bambas (:mayhemer)]

Michal is an assignee.

Comment 7

[Nhi Nguyen (:nhi)]

as discussed, putting on :mayhemer's radar if he can get to it this cycle.

Comment 8

[Honza Bambas (:mayhemer)]

Luke I need some confirmations first. Is the place you call the API at [1] where mCacheInfo is obtained at [2] and it all happens on the content process? Does it end up at [3] on the parent process?

If it's all true then this is getting more complicated than Michal claimed and I may not have more time to work on this soon.

[1] https://searchfox.org/mozilla-central/rev/c8dba8c886609d0fdb8fae68bab865bf03f1165c/dom/script/ScriptLoader.cpp#2972
[2] https://searchfox.org/mozilla-central/rev/c8dba8c886609d0fdb8fae68bab865bf03f1165c/dom/script/ScriptLoadHandler.cpp#416
[3] https://searchfox.org/mozilla-central/rev/c8dba8c886609d0fdb8fae68bab865bf03f1165c/netwerk/ipc/NeckoParent.cpp#352

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: DRAFT: Bug 1554652 - Make nsICacheEntry.openAlternativeOutputStream return the output stream optionally asynchronously via a callback

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 9089501 [details]
DRAFT: Bug 1554652 - Make nsICacheEntry.openAlternativeOutputStream return the output stream optionally asynchronously via a callback

Luke, please check if this is (in the direction of) what you want and give it some testing. I want to write an automated test too, but it will be some work.

Comment 11

[Luke Wagner [:luke]]

So for context: this bug is for fixing a pre-existing race condition that currently leads to a rare intermittent failure with the current usage of alt-data by the JS bytecode cache. However, the additional patches baku wrote in bug 1487113 make this race happen much more frequently in new tests, leading to the backout of his patches. (baku's patches are, in turn, prequisites for the wasm alt-data patches.)

I'm afraid I don't understand this area of the code to answer your questions, though. baku: do you know the answer to Honza's question in comment 8 and whether the approach in comment 10 is what is needed?

Comment 12

[Honza Bambas (:mayhemer)]

So It's the two patches to apply and test with them?

D26731 Bug 1487113 - Use alt-data to cache stream-compiled WebAssembly modules (r=baku)
D27101 Bug 1487113 - Add pref javascript.options.wasm_caching (r=baku)

Comment 13

[Luke Wagner [:luke]]

No, those are my dependent patches. baku's two patches are the ones that landed in bug 1487113 comment 78.

Comment 14

[Honza Bambas (:mayhemer)]

"Dependent" means that this bug has to be fixed to land them? Are the tests in those "dependent" patches utilize the required functionality and fail (intermitentntly) w/o it? I simply want a test env and the starting point (where to use the new async API, if anywhere) to finish this work.

Comment 15

[Luke Wagner [:luke]]

To be clear: this bug has to be fixed to re-land bug 1487113 comment 78 which is needed to land my wasm caching patches. If you look at the backout comment (bug 1487113 comment 80), you can see which tests start failing with the bug 1487113 comment 78 patches, which I think you could use to test your patches in this bug.

Comment 16

[Honza Bambas (:mayhemer)]

Thanks, Luke.

Comment 17

[Honza Bambas (:mayhemer)]

The think that blocks me here is that I need to find the place where the new/modified API is to be used and who is going to be the implementer of the callback. Then have a test to check this functionality actually works.

Comment 18

[Luke Wagner [:luke]]

For baku to be able to re-land the patches in bug 1487113 comment 78, iiuc, the usage of nsICacheInfoChanel.openAlternativeOutputStream() that needs to be updated is the one in ScriptLoader.cpp, where your patch has "TODO": that site goes from intermittently failing to perma-failing with baku's patches. So if you can (1) fill in that TODO, (2) get a clean try run (specifically for the test failures in bug 1487113 comment 80), (3) land all these patches, then I'm good to go to land wasm caching. Make sense?

Comment 19

[Honza Bambas (:mayhemer)]

About status: I rebased the patches and pushed to try again - Thu, Oct 3, 15:41:48. I can reproduce the problems reported in Comment 80 of the blocked bug. My next step is to find out (with some added logging) what happens.

Comment 20

[Honza Bambas (:mayhemer)]

Bug 1487113 is now P3. Reflecting the priority here as well and dis-assigning myself. I will just update the patches that I rebased in bug 1487113.

Comment 21

[Ahmed Sobeh]

Blocked bug 1487113 bumped to P2, does that help with prioritizing this?

Ta in advance

Comment 22

[Honza Bambas (:mayhemer)]

The task here is:

• push to try these (rebased):
  • (1562) ⚙ D25518 Bug 1487113 - nsICacheInfoChannel.originalInputStream as attribute, r?valentin
  • (1562) ⚙ D31791 Bug 1487113 - nsICacheInfoChannel.alternativeDataInputStream as attribute, r=valentin
• try to repro the failures in bug 1487113 comment 80
• try to fix them with the patch for this bug

Comment 23

[Honza Bambas (:mayhemer)]

Try push results:

TEST-UNEXPECTED-FAIL | browser/components/newtab/test/browser/abouthomecache/browser_basic_endtoend.js | Uncaught exception - undefined - threw exception: [Exception... "Component returned failure code: 0x80470002 (NS_BASE_STREAM_CLOSED) [nsIInputStream.available]" nsresult: "0x80470002 (NS_BASE_STREAM_CLOSED)" location: "JS frame :: chrome://mochitests/content/browser/browser/components/newtab/test/browser/abouthomecache/head.js ::

netwerk/test/unit_ipc/test_alt-data_stream_wrap.js times out because the input streams leak, bug 1653996.

Comment 24

[Valentin Gosu [:valentin]]

This isn't blocking important work anymore.