Last Comment Bug 716293 - IO on the main thread caused by nsDiskCacheOutputStream::Close()
: IO on the main thread caused by nsDiskCacheOutputStream::Close()
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla12
Assigned To: Michal Novotny (:michal)
:
Mentors:
Depends on:
Blocks: 513074 715774
  Show dependency treegraph
 
Reported: 2012-01-07 16:33 PST by Michal Novotny (:michal)
Modified: 2012-02-07 19:20 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix - release sink on the same thread where the data is written (1.69 KB, patch)
2012-01-07 16:37 PST, Michal Novotny (:michal)
no flags Details | Diff | Review
patch v2 - added a comment (1.77 KB, patch)
2012-01-09 08:35 PST, Michal Novotny (:michal)
bjarne: review+
Details | Diff | Review

Description Michal Novotny (:michal) 2012-01-07 16:33:29 PST
nsDiskCacheOutputStream::Close() is called on the main thread if all nsInputStreamTeeWriteEvent events finish before nsStreamListenerTee::OnStartRequest() is called.
Comment 1 Michal Novotny (:michal) 2012-01-07 16:37:35 PST
Created attachment 586742 [details] [diff] [review]
fix - release sink on the same thread where the data is written
Comment 2 Bjarne (:bjarne) 2012-01-09 04:27:06 PST
Comment on attachment 586742 [details] [diff] [review]
fix - release sink on the same thread where the data is written

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

Looks reasonable - does it pass tryserver? Is there a way we can have a test for this?

Please re-request review after addressing the above questions.

::: netwerk/base/src/nsStreamListenerTee.cpp
@@ +67,5 @@
>      if (mInputTee) {
>          mInputTee->SetSink(nsnull);
>          mInputTee = 0;
>      }
> +

Nit: Please add a comment including the bug-number.
Comment 3 Michal Novotny (:michal) 2012-01-09 08:35:51 PST
Created attachment 587008 [details] [diff] [review]
patch v2 - added a comment

(In reply to Bjarne (:bjarne) from comment #2)
> Looks reasonable - does it pass tryserver?

No, it doesn't. But tryserver is useless these days.

result with patch: https://tbpl.mozilla.org/?tree=Try&rev=96d185e8731e
result without it: https://tbpl.mozilla.org/?tree=Try&rev=bd7e5b7b5ea3


> Is there a way we can have a test for this?

I don't see a way how to test it.
Comment 4 Bjarne (:bjarne) 2012-01-09 13:29:51 PST
Comment on attachment 587008 [details] [diff] [review]
patch v2 - added a comment

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

Code looks ok. Would be more happy if it passed try and/or had a test, but fair enough...
Comment 5 Michal Novotny (:michal) 2012-01-11 09:20:05 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/dc23947abad9
Comment 6 Matt Brubeck (:mbrubeck) 2012-01-12 08:36:44 PST
https://hg.mozilla.org/mozilla-central/rev/dc23947abad9

Note You need to log in before you can comment on or make changes to this bug.