Closed
Bug 939567
Opened 12 years ago
Closed 12 years ago
HTTP cache v2: nsWyciwygChannel must open cache entry on a single thread / intermittent test_bug172261.html, test_bug255820.html, test_bug715739.html, browser_bug822367.js | Test timed out.
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(2 files)
894.85 KB,
text/plain
|
Details | |
3.89 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
Happens mostly on osx opt builds.
https://tbpl.mozilla.org/?tree=Gum&rev=cd8fc27f67b6
https://tbpl.mozilla.org/?tree=Gum&rev=41ba8a1e752f
/tests/parser/htmlparser/tests/mochitest/test_bug715739.html
/tests/content/html/document/test/test_bug255820.html
/tests/content/html/document/test/test_bug172261.html
Only manifests with cache2=on. For now hard to track, options:
- run the individual tests with NSPR logs directly on try machines (hard)
- run the produced dist locally (I cannot build the with identical mozconfig as try is building)
![]() |
Assignee | |
Updated•12 years ago
|
Keywords: intermittent-failure
Summary: HTTP cache v2: With cache2=on: intermittent test_bug172261.html, test_bug255820.html, test_bug715739.html | Test timed out. → ONLY WITH HTTP CACHE v2 TURNED ON: intermittent test_bug172261.html, test_bug255820.html, test_bug715739.html, browser_bug822367.js | Test timed out.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
![]() |
Assignee | |
Comment 6•12 years ago
|
||
Comment on attachment 8358473 [details]
nspr log from manual talos run (822367)
browser_bug822367.js (and I strongly believe all other timing-out tests):
sometimes it happens that nsWyciwygChannel::AsyncOpen for the same URL is called sooner on the main thread then nsWyciwygChannel::WriteToCacheEntryInternal on the IO thread.
hence, we open (and create for the first time) the entry for reading BEFORE it has even been created for writing on the IO thread.
the reading channel gets ENTRY_NOT_FOUND as a result because nsWyciwygChannel::WriteToCacheEntryInternal causes the entry pending its load be doomed (OPEN_TRUNCATE) - bug 923688.
Possible solutions:
- wyciwyg must use the async API on either the main thread or IO thread for both write and read to preserve opening order.
- bug 923688 would fix this, but for me it's somewhat disputable whether it is a true solution in fact, but probably is
Also, regarding bug 923688, we should have some general solution for this concurrent open read/open write anyway and that bug might be that solution.
Michal, what do you think?
Flags: needinfo?(michal.novotny)
![]() |
Assignee | |
Comment 7•12 years ago
|
||
We clearly need both. When wyciwyg channel wants to read (is navigated) the entry must already be there to read, even when it should happen concurrently (which is fully available in this case).
So, we need to move opening the cache entry for read either the IO thread or open even for write on the main thread and buffer the data (string array probably to avoid buffer copy). But after the old cache is removed, we can just open instantly. For truncate opens the callback is expected synchronously.
![]() |
Assignee | |
Updated•12 years ago
|
Keywords: intermittent-failure
Summary: ONLY WITH HTTP CACHE v2 TURNED ON: intermittent test_bug172261.html, test_bug255820.html, test_bug715739.html, browser_bug822367.js | Test timed out. → HTTP cache v2: nsWyciwygChannel must open cache entry on a single thread / intermittent test_bug172261.html, test_bug255820.html, test_bug715739.html, browser_bug822367.js | Test timed out.
![]() |
Assignee | |
Updated•12 years ago
|
Status: NEW → ASSIGNED
Comment 8•12 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #6)
> hence, we open (and create for the first time) the entry for reading BEFORE
> it has even been created for writing on the IO thread.
IIUC this happens only when CacheIndex patch is applied, right? The reason, why opening entry for reading finishes before it is created by write request, is that CacheIndex::HasEntry() returns DOES_NOT_EXIST. Do I understand the problem correctly?
Flags: needinfo?(michal.novotny)
![]() |
Assignee | |
Comment 9•12 years ago
|
||
(In reply to Michal Novotny (:michal) from comment #8)
> (In reply to Honza Bambas (:mayhemer) from comment #6)
> > hence, we open (and create for the first time) the entry for reading BEFORE
> > it has even been created for writing on the IO thread.
>
> IIUC this happens only when CacheIndex patch is applied, right?
No, this was happening since I was first testing the wyciwyg patch, long ago we even decided to write an index.
> The reason,
> why opening entry for reading finishes before it is created by write
> request, is that CacheIndex::HasEntry() returns DOES_NOT_EXIST. Do I
> understand the problem correctly?
I think you don't understand or I don't explain well. Another try:
- a script does document.write(); document.close();
- this fires wyciwigchanell::writeinternal event with the string as an argument (this event opens the entry with TRUNCATE - on the IO thread, but this event execution on the IO thread is delayed
- immediately after the document.close() the browser navigates to the same document via history navigation ; the url in bfcache is already wyciwig://0/www.example.org/, so it does wyciwigchannel::asyncopen that calls asyncopen(READONLY) for the the "wyciwig://0/www.example.org/" cache entry
- entry at that moment doesn't even exist, since IO thread hasn't fired the event yet
This has nothing to do with the index.
![]() |
Assignee | |
Comment 10•12 years ago
|
||
Attachment #8359984 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 11•12 years ago
|
||
Looks alright.
![]() |
Assignee | |
Updated•12 years ago
|
Attachment #8359984 -
Attachment description: v1 → v1 [healthy]
Comment 12•12 years ago
|
||
Comment on attachment 8359984 [details] [diff] [review]
v1 [healthy]
Review of attachment 8359984 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp
@@ +466,5 @@
>
> + if (mozilla::net::CacheObserver::UseNewCache()) {
> + mozilla::DebugOnly<nsresult> rv = EnsureWriteCacheEntry();
> + // If this fails in release, that is not much of a deal. We try
> + // it ones again on the IO thread.
ones -> once
Attachment #8359984 -
Flags: review?(michal.novotny) → review+
![]() |
Assignee | |
Comment 13•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•