Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
374063002:
[fsp] Fix crash because of calling net::GetMimeTypeFromFile on UI thread. (Closed)
|
Created:
6 years, 5 months ago by mtomasz Modified:
6 years, 5 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[fsp] Fix crash because of calling net::GetMimeTypeFromFile on UI thread.
This patch fixes the issue by calling net::GetMimeTypeFromFile on the blocking
pool.
TEST=unit_tests: *FileHandlersMimeUtilTest*"
BUG=392072
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282048
Patch Set 1 #
Total comments: 10
Patch Set 2 : Fixed a comment. #
Messages
Total messages: 19 (0 generated)
@kinaba: PTAL. Thanks.
lgtm
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/374063002/1
@kalman: PTAL since @benwells is OOO. Thanks.
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util.cc:65: // MIME type not available with metadata, hence try to guess is from the s/is/it https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util.cc:69: mime_type_from_extension.get(); this pattern is such a pain :( it seems like it's not a great fit for PostBlockingPoolTaskAndReply. would it be easier to just use PostBlockingPoolTask and manage the fetch/reply yourself? i.e. basically a much simple async-ifying of all of this logic? i.e. make GetMimeTypeForLocalPath post to the blocking pool with a method like GetMimeTypeForLocalPathOnBlockingPool which is basically GetMimeTypeForLocalPath? https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc:129: // flush it at least 6 times. This is unelegant, but there seem to be no inelegant and indeed. it would be nice if FlushForTesting returned whether anything was flushed but I can't see any candidates. it's a pity that the blocking pools don't just use the runloop for testing. is there a way to arrange that? any chance it magically already happens? I presume not, otherwise (i < 2) condition would work.
https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util.cc:65: // MIME type not available with metadata, hence try to guess is from the On 2014/07/08 16:02:28, kalman wrote: > s/is/it Done. https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util.cc:69: mime_type_from_extension.get(); On 2014/07/08 16:02:28, kalman wrote: > this pattern is such a pain :( it seems like it's not a great fit for > PostBlockingPoolTaskAndReply. would it be easier to just use > PostBlockingPoolTask and manage the fetch/reply yourself? > > i.e. basically a much simple async-ifying of all of this logic? > > i.e. make GetMimeTypeForLocalPath post to the blocking pool with a method like > GetMimeTypeForLocalPathOnBlockingPool which is basically > GetMimeTypeForLocalPath? I'm not a huge fan of it either, but this pattern is often used for Post*TaskAndReply() in Chromium code and I've been asked several times by reviewers to go this way rather than post the answer back manually. WDYT? https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc:129: // flush it at least 6 times. This is unelegant, but there seem to be no On 2014/07/08 16:02:28, kalman wrote: > inelegant > > and indeed. > > it would be nice if FlushForTesting returned whether anything was flushed but I > can't see any candidates. it's a pity that the blocking pools don't just use the > runloop for testing. is there a way to arrange that? any chance it magically > already happens? I presume not, otherwise (i < 2) condition would work. I just found a utility function in drive code: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... I think we could upstream and reuse also here. WDYT?
lgtm https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util.cc:69: mime_type_from_extension.get(); On 2014/07/09 02:21:02, mtomasz wrote: > On 2014/07/08 16:02:28, kalman wrote: > > this pattern is such a pain :( it seems like it's not a great fit for > > PostBlockingPoolTaskAndReply. would it be easier to just use > > PostBlockingPoolTask and manage the fetch/reply yourself? > > > > i.e. basically a much simple async-ifying of all of this logic? > > > > i.e. make GetMimeTypeForLocalPath post to the blocking pool with a method like > > GetMimeTypeForLocalPathOnBlockingPool which is basically > > GetMimeTypeForLocalPath? > > I'm not a huge fan of it either, but this pattern is often used for > Post*TaskAndReply() in Chromium code and I've been asked several times by > reviewers to go this way rather than post the answer back manually. > > WDYT? alright. but let it be on the record that I think ^ is a much simpler transformation. https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc:129: // flush it at least 6 times. This is unelegant, but there seem to be no On 2014/07/09 02:21:02, mtomasz wrote: > On 2014/07/08 16:02:28, kalman wrote: > > inelegant > > > > and indeed. > > > > it would be nice if FlushForTesting returned whether anything was flushed but > I > > can't see any candidates. it's a pity that the blocking pools don't just use > the > > runloop for testing. is there a way to arrange that? any chance it magically > > already happens? I presume not, otherwise (i < 2) condition would work. > > I just found a utility function in drive code: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > I think we could upstream and reuse also here. WDYT? hey that would be good. how do you propose to reuse it?
https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc:129: // flush it at least 6 times. This is unelegant, but there seem to be no On 2014/07/09 02:38:20, kalman wrote: > On 2014/07/09 02:21:02, mtomasz wrote: > > On 2014/07/08 16:02:28, kalman wrote: > > > inelegant > > > > > > and indeed. > > > > > > it would be nice if FlushForTesting returned whether anything was flushed > but > > I > > > can't see any candidates. it's a pity that the blocking pools don't just use > > the > > > runloop for testing. is there a way to arrange that? any chance it magically > > > already happens? I presume not, otherwise (i < 2) condition would work. > > > > I just found a utility function in drive code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > I think we could upstream and reuse also here. WDYT? > > hey that would be good. how do you propose to reuse it? We could move that code from test_util.h|cc to a newly created content/public/test/blocking_pool_runner.h|cc. We could then call RunBlockingPoolTasksUntilIdle() here, instead of the unelegant loop. I think we could also reuse it in other places. In Chromium code FlushForTesting is often called twice for the same reason. https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ses... https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/br... And possibly tens of other places. If you are fine with it, I'd like to do it in a separate patch. WDYT?
https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc (right): https://codereview.chromium.org/374063002/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/file_handlers/mime_util_unittest.cc:129: // flush it at least 6 times. This is unelegant, but there seem to be no On 2014/07/09 03:55:42, mtomasz wrote: > On 2014/07/09 02:38:20, kalman wrote: > > On 2014/07/09 02:21:02, mtomasz wrote: > > > On 2014/07/08 16:02:28, kalman wrote: > > > > inelegant > > > > > > > > and indeed. > > > > > > > > it would be nice if FlushForTesting returned whether anything was flushed > > but > > > I > > > > can't see any candidates. it's a pity that the blocking pools don't just > use > > > the > > > > runloop for testing. is there a way to arrange that? any chance it > magically > > > > already happens? I presume not, otherwise (i < 2) condition would work. > > > > > > I just found a utility function in drive code: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > > > I think we could upstream and reuse also here. WDYT? > > > > hey that would be good. how do you propose to reuse it? > > We could move that code from test_util.h|cc to a newly created > content/public/test/blocking_pool_runner.h|cc. > > We could then call > RunBlockingPoolTasksUntilIdle() here, instead of the unelegant loop. I think we > could also reuse it in other places. In Chromium code FlushForTesting is often > called twice for the same reason. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ses... > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > https://code.google.com/p/chromium/codesearch#chromium/src/components/nacl/br... > > And possibly tens of other places. > > If you are fine with it, I'd like to do it in a separate patch. WDYT? sounds good.
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/374063002/20001
The CQ bit was unchecked by mtomasz@chromium.org
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/374063002/20001
Message was sent while issue was closed.
Change committed as 282048 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
