(In reply to Nathan Froyd [:froydnj] from comment #9)
(In reply to :Gijs (he/him) from comment #7)
Having just commented on bug 1163021 comment 22, I remembered that one other option I considered was to have the jar log code dispatch a runnable to the main thread for each jar access that we (may) want to log... but that's quite a few of those runnables and seems a bit of a waste... am I missing an obvious simpler solution?
The other alternative is to buffer all the data under a mutex, and only write things out when the zip archive is closed.
I'm not too exercised about codepaths that only get exercised
I see what you did there! :-)
under PGO generate runs, as we're not "shipping" this code and as long as they're not blatantly inefficient.
Also, dumb question, if different threads can hit this code, and they all write to this one file from their threads... that's not threadsafe either, right? (ie, the ordering file could be a mess...).
Um, yes, that seems bad. Are we really touching jar things from more than two threads (main and something else, though that's already bad...)?
I gotta admit, I didn't do any runtime logging/checks, but I'm going on the following code inspection / observations:
- we're definitely touching this from some thread other than the mainthread (the assertion stack in comment #0 shows this) - and we do this quite early / regularly, as even the initial "start firefox to prep a profile and then exit" run of the PGO generate run fails with this assertion. At a guess, I suspect this has happened since at least bug 1373708.
- I can see a direct callpath to
Open method, which is IDL exposed so all JS accesses are guaranteed to be main-thread because it's JS-based XPCOM, as well as having several C++ callers that look like they should be mainthread (dom/xhr/XMLHttpRequestMainThread.cpp sure sounds like that, anyway, and I'm pretty sure the main omni.ja init things in xpcom/build/Omnijar.cpp are, too) - we're just hoping none of them get hit at the same time, AFAICT... Basically, anything that touches zip files and doesn't directly run through nsJARChannels or the URI preloader
- On that last note, I'm fairly sure the URL preloader (cf here) and jar channels (cf. here, which I think is what the stack in comment #0 hits) each use their own background thread, ie those are probably different, too.
OpenArchive, we're calling
Init on the
zipLog, which guards on
fd, so if we're lucky then that gets called from some thread and finishes and writes to
fd before any other thread calls into it (ie hopefully the first read from an archive is somewhere that blocks enough stuff that it is guaranteed to have finished before any further reads). In terms of the actual writes, I think they're all from here: https://searchfox.org/mozilla-central/rev/053579099d936e26393ac10b809b14fb5841c0f0/modules/libjar/nsZipArchive.cpp#477 . The URLPreloader definitely calls
GetItem on non-mainthreads, as does the jar channel read mentioned above. XMLHttpRequestMainThread looks like it calls it on the main thread, and omni.ja exposes the zipreaders it opens and callsites like prefs I'm pretty sure also access those items from the main thread, through the URL preloader, that is, if the URL preloader doesn't have the data preloaded at that point, it'll go read it from the "normal" location, AIUI.
But honestly, if we are touching the logger from multiple threads, it seems like we would be touching all of
nsZipArchive from multiple threads, which is even more terrible...so maybe we should stop doing that?
Well, I hope/assume that an individual nsZipArchive instance is never shared across more than 1 thread, but the jar logger (
static ZipArchiveLogger zipLog) is a static, so it is / can be accessed from multiple zip archives if I'm not mistaken?
Anyway, assuming I'm right about this (and please check my reasoning!)... what to do. Looking at this some more, while the init and determination of whether we want to jar-log the file is made when calling
OpenArchive, the actual writing is done for each
GetItem call. So AIUI we'd want to dispatch to mainthread from
GetItem, and hand the runnable thing we're dispatching a copy of the archive's nsIFile and the path we're reading. The jar log could cache the gredir the first time it's called and that might make things OK (assuming that doesn't sound too "blatantly inefficient" :-) ). Does that sound right? If so I can probably do a patch for that...
If we're worried about the zip archive instances being reused across threads, I'm happy to file a bug on that but I'm so many yaks away from home that I'd prefer if someone else did the actual shaving (with apologies for stretching that metaphor).