Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2182213004:
Use ChildThreadImpl::thread_safe_sender in font proxy if available (Closed)
|
Created:
4 years, 4 months ago by Ilya Kulshin Modified:
4 years, 4 months ago CC:
chromium-reviews, darin-cc_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse ChildThreadImpl::thread_safe_sender in font proxy if available
This changes the logic that the font proxy uses to get the IPC
sender. It will now try to get a sender in the initialization code,
and use that one if available. This allows it to get the sender
while it is still running on the main thread in the plugin, so that
it can use it for IPCs later when called from a different thread.
This is a threading violation, but it will allow the proxy to work
when called by Flash from a background thread.
BUG=631254
Committed: https://crrev.com/ddaf9994ab75ff143da0c2f5c44499e49db1815f
Cr-Commit-Position: refs/heads/master@{#408551}
Patch Set 1 #Patch Set 2 : Fix unittests #Patch Set 3 : Rebase #Patch Set 4 : Fix unittests #
Total comments: 2
Patch Set 5 : Refactor to make cherrypick easier by reducing conflicts #Patch Set 6 : Set thread_safe_sender in font proxy init, if available. #
Total comments: 5
Messages
Total messages: 48 (26 generated)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
Description was changed from ========== Revert "Switch to using ChildThread::Get() to get the IPC sender" This reverts commit 0a6c181cc466a4b4f8ade9b1e633992378df4276. BUG= ========== to ========== Revert "Switch to using ChildThread::Get() to get the IPC sender" This allows the font proxy to use different logic to obtain the IPC sender, depending on whether it is running in the renderer or the plugin process. When running in the renderer, it will use ChildThread::Get, as before. When running in the plugin, it will now use PluginGlobals::GetBrowserSender(). This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. This reverts commit 0a6c181cc466a4b4f8ade9b1e633992378df4276. BUG=631254 ==========
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kulshin@chromium.org changed reviewers: + forshaw@chromium.org, jam@chromium.org, wfh@chromium.org
ptal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping. Can I get a review so this can make it into tomorrow's canary?
I'm sorry I do not know the impact of this change, jam@ reviewed the original CL so perhaps he is best placed to know what this would do...?
rockot@chromium.org changed reviewers: + rockot@chromium.org
While I could understand if we want to just land this revert as-is since it's at least more stable the current situation, it doesn't seem like it would be very challenging implement a one-off IPC::Sender in ppapi_plugin_main which does an intermediate PostTask to the correct thread. Is there some subtle reason not to do this?
On 2016/07/28 15:28:31, Ken Rockot wrote: > While I could understand if we want to just land this revert as-is since it's at > least more stable the current situation, it doesn't seem like it would be very > challenging implement a one-off IPC::Sender in ppapi_plugin_main which does an > intermediate PostTask to the correct thread. Is there some subtle reason not to > do this? It lgtm as a quick fix, of course it would be nice to fix it properly somewhere down the line.
https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (left): https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:351: return sender_override_ ? sender_override_ : ChildThread::Get(); you can fix this by changing ChildThread::Get() to ChildThreadImpl::current()->thread_safe_sender()
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (left): https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:351: return sender_override_ ? sender_override_ : ChildThread::Get(); On 2016/07/28 16:34:31, jam wrote: > you can fix this by changing > > ChildThread::Get() > to > ChildThreadImpl::current()->thread_safe_sender() thread_safe_sender still needs to be called from the main thread, since it obtains the sender from tls. At the point where I know this code is running from the main thread (initialization) the IPC system is not yet fully functional, so I cannot get the sender at that time. By the time the code gets here, it can no longer be relied to be on the main thread, so thread_safe_sender will not work.
On 2016/07/28 at 19:01:13, kulshin wrote: > https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... > File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (left): > > https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... > content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:351: return sender_override_ ? sender_override_ : ChildThread::Get(); > On 2016/07/28 16:34:31, jam wrote: > > you can fix this by changing > > > > ChildThread::Get() > > to > > ChildThreadImpl::current()->thread_safe_sender() > > thread_safe_sender still needs to be called from the main thread, since it obtains the sender from tls. At the point where I know this code is running from the main thread (initialization) the IPC system is not yet fully functional, so I cannot get the sender at that time. By the time the code gets here, it can no longer be relied to be on the main thread, so thread_safe_sender will not work. By the time InitializeDWriteFontProxy is called in PpapiPluginMain, ChildThreadImpl::current()->thread_safe_sender() is definitely valid - it's initialized from ChildThreadImpl::Init which runs when constructing the PpapiThread. This pointer can be held onto and used later from any thread.
Description was changed from ========== Revert "Switch to using ChildThread::Get() to get the IPC sender" This allows the font proxy to use different logic to obtain the IPC sender, depending on whether it is running in the renderer or the plugin process. When running in the renderer, it will use ChildThread::Get, as before. When running in the plugin, it will now use PluginGlobals::GetBrowserSender(). This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. This reverts commit 0a6c181cc466a4b4f8ade9b1e633992378df4276. BUG=631254 ========== to ========== Use ChildThreadImpl::thread_safe_sender in font proxy if available This changes the logic that the font proxy uses to get the IPC sender. It will now try to get a sender in the initialization code, and use that one if available. This allows it to get the sender while it is still running on the main thread in the plugin, so that it can use it for IPCs later when called from a different thread. This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. BUG=631254 ==========
On 2016/07/28 19:11:41, Ken Rockot wrote: > On 2016/07/28 at 19:01:13, kulshin wrote: > > > https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... > > File content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc (left): > > > > > https://codereview.chromium.org/2182213004/diff/60001/content/child/dwrite_fo... > > content/child/dwrite_font_proxy/dwrite_font_proxy_win.cc:351: return > sender_override_ ? sender_override_ : ChildThread::Get(); > > On 2016/07/28 16:34:31, jam wrote: > > > you can fix this by changing > > > > > > ChildThread::Get() > > > to > > > ChildThreadImpl::current()->thread_safe_sender() > > > > thread_safe_sender still needs to be called from the main thread, since it > obtains the sender from tls. At the point where I know this code is running from > the main thread (initialization) the IPC system is not yet fully functional, so > I cannot get the sender at that time. By the time the code gets here, it can no > longer be relied to be on the main thread, so thread_safe_sender will not work. > > By the time InitializeDWriteFontProxy is called in PpapiPluginMain, > ChildThreadImpl::current()->thread_safe_sender() is definitely valid - it's > initialized from ChildThreadImpl::Init which runs when constructing the > PpapiThread. This pointer can be held onto and used later from any thread. Hmm, indeed it is, it seems the fonts are initialized later during startup in the plugin than in the renderer.
ptal. I refactored the change to use ChildThreadImpl::thread_safe_sender, which also allowed me to remove most of the code and keep the changes just in the init logic.
The CQ bit was checked by kulshin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack for crbug.com/631254: set the sender if we can get one, so that when nit: saying hack makes it sound like this should be fixed properly. using thread-safe sender is a standard pattern. I would just get rid of this comment since it's documenting what the code is doing. https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:84: if (!sender && ChildThreadImpl::current()) are you sure you need the ChildThreadImpl::current() check?
https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack for crbug.com/631254: set the sender if we can get one, so that when On 2016/07/28 22:04:43, jam wrote: > nit: saying hack makes it sound like this should be fixed properly. using > thread-safe sender is a standard pattern. > > I would just get rid of this comment since it's documenting what the code is > doing. This should be fixed properly. One possible fix we are talking about is to detect when the pepper font api is being called from the wrong thread, and marshal the call to the appropriate thread. https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:84: if (!sender && ChildThreadImpl::current()) On 2016/07/28 22:04:43, jam wrote: > are you sure you need the ChildThreadImpl::current() check? Yes, otherwise it crashes in the renderer because ChildThreadImpl::current() returns null at the time this code is initialized.
ananta@chromium.org changed reviewers: + ananta@chromium.org
lgtm
The CQ bit was checked by kulshin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from forshaw@chromium.org Link to the patchset: https://codereview.chromium.org/2182213004/#ps120001 (title: "Set thread_safe_sender in font proxy init, if available.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use ChildThreadImpl::thread_safe_sender in font proxy if available This changes the logic that the font proxy uses to get the IPC sender. It will now try to get a sender in the initialization code, and use that one if available. This allows it to get the sender while it is still running on the main thread in the plugin, so that it can use it for IPCs later when called from a different thread. This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. BUG=631254 ========== to ========== Use ChildThreadImpl::thread_safe_sender in font proxy if available This changes the logic that the font proxy uses to get the IPC sender. It will now try to get a sender in the initialization code, and use that one if available. This allows it to get the sender while it is still running on the main thread in the plugin, so that it can use it for IPCs later when called from a different thread. This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. BUG=631254 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Use ChildThreadImpl::thread_safe_sender in font proxy if available This changes the logic that the font proxy uses to get the IPC sender. It will now try to get a sender in the initialization code, and use that one if available. This allows it to get the sender while it is still running on the main thread in the plugin, so that it can use it for IPCs later when called from a different thread. This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. BUG=631254 ========== to ========== Use ChildThreadImpl::thread_safe_sender in font proxy if available This changes the logic that the font proxy uses to get the IPC sender. It will now try to get a sender in the initialization code, and use that one if available. This allows it to get the sender while it is still running on the main thread in the plugin, so that it can use it for IPCs later when called from a different thread. This is a threading violation, but it will allow the proxy to work when called by Flash from a background thread. BUG=631254 Committed: https://crrev.com/ddaf9994ab75ff143da0c2f5c44499e49db1815f Cr-Commit-Position: refs/heads/master@{#408551} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ddaf9994ab75ff143da0c2f5c44499e49db1815f Cr-Commit-Position: refs/heads/master@{#408551}
Message was sent while issue was closed.
https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack for crbug.com/631254: set the sender if we can get one, so that when On 2016/07/28 22:10:41, Ilya Kulshin wrote: > On 2016/07/28 22:04:43, jam wrote: > > nit: saying hack makes it sound like this should be fixed properly. using > > thread-safe sender is a standard pattern. > > > > I would just get rid of this comment since it's documenting what the code is > > doing. > > This should be fixed properly. One possible fix we are talking about is to > detect when the pepper font api is being called from the wrong thread, and > marshal the call to the appropriate thread. I don't understand this comment. That's what the SafeSender is doing internally?
Message was sent while issue was closed.
On 2016/07/29 22:34:58, jam wrote: > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack for > crbug.com/631254: set the sender if we can get one, so that when > On 2016/07/28 22:10:41, Ilya Kulshin wrote: > > On 2016/07/28 22:04:43, jam wrote: > > > nit: saying hack makes it sound like this should be fixed properly. using > > > thread-safe sender is a standard pattern. > > > > > > I would just get rid of this comment since it's documenting what the code is > > > doing. > > > > This should be fixed properly. One possible fix we are talking about is to > > detect when the pepper font api is being called from the wrong thread, and > > marshal the call to the appropriate thread. > > I don't understand this comment. That's what the SafeSender is doing internally? The SafeSender will make sure the IPC runs from the right thread, correct? The problem is that there's a bunch of non-IPC code that runs before it even gets to the point where it knows it needs to send an IPC, and that code also is not being called from the right thread. That is the code that I'm talking about needing to move to a different thread (but not in this patch).
Message was sent while issue was closed.
On 2016/07/29 22:38:29, Ilya Kulshin wrote: > On 2016/07/29 22:34:58, jam wrote: > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack for > > crbug.com/631254: set the sender if we can get one, so that when > > On 2016/07/28 22:10:41, Ilya Kulshin wrote: > > > On 2016/07/28 22:04:43, jam wrote: > > > > nit: saying hack makes it sound like this should be fixed properly. using > > > > thread-safe sender is a standard pattern. > > > > > > > > I would just get rid of this comment since it's documenting what the code > is > > > > doing. > > > > > > This should be fixed properly. One possible fix we are talking about is to > > > detect when the pepper font api is being called from the wrong thread, and > > > marshal the call to the appropriate thread. > > > > I don't understand this comment. That's what the SafeSender is doing > internally? > > The SafeSender will make sure the IPC runs from the right thread, correct? Correct> > The > problem is that there's a bunch of non-IPC code that runs before it even gets to > the point where it knows it needs to send an IPC, and that code also is not > being called from the right thread. I don't know what code you're talking about. But generally the pattern is on startup we save a pointer to safe sender and then call it later on different threads. Can't that be done here as well? > That is the code that I'm talking about > needing to move to a different thread (but not in this patch).
Message was sent while issue was closed.
On 2016/07/29 23:14:08, jam wrote: > On 2016/07/29 22:38:29, Ilya Kulshin wrote: > > On 2016/07/29 22:34:58, jam wrote: > > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > > File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc (right): > > > > > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > > content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack > for > > > crbug.com/631254: set the sender if we can get one, so that when > > > On 2016/07/28 22:10:41, Ilya Kulshin wrote: > > > > On 2016/07/28 22:04:43, jam wrote: > > > > > nit: saying hack makes it sound like this should be fixed properly. > using > > > > > thread-safe sender is a standard pattern. > > > > > > > > > > I would just get rid of this comment since it's documenting what the > code > > is > > > > > doing. > > > > > > > > This should be fixed properly. One possible fix we are talking about is to > > > > detect when the pepper font api is being called from the wrong thread, and > > > > marshal the call to the appropriate thread. > > > > > > I don't understand this comment. That's what the SafeSender is doing > > internally? > > > > The SafeSender will make sure the IPC runs from the right thread, correct? > > Correct> > > > The > > problem is that there's a bunch of non-IPC code that runs before it even gets > to > > the point where it knows it needs to send an IPC, and that code also is not > > being called from the right thread. > > I don't know what code you're talking about. But generally the pattern is on > startup we save a pointer to safe sender and then call it later on different > threads. Can't that be done here as well? > Consider the callstack in https://bugs.chromium.org/p/chromium/issues/detail?id=631254#c3. This is happening during process runtime, and happens to be not on the main thread. At the top of the stack is DWriteFontCollectionProxy, which actually calls the IPC code. Before that there is Skia code, and before that there is blink code. Although the code was crashing because the font proxy was unable to obtain a sender, the blink code is also triggering DCHECKs because it is being called on the wrong thread. The blink code is the code that I want to marshal to a different thread (and therefore also the Skia and font proxy code that blink will call). That marshaling will need to happen in content::BrowserFontResource_Trusted::Describe as well as any other similar functions that are called by the plugin code. As far as saving a pointer to the safe sender and then calling it later on different threads, that is exactly what this patch implements. It is still not sufficient, because by the time it gets to the point where it needs the sender, we are already running code on the wrong thread.
Message was sent while issue was closed.
On 2016/07/30 00:25:20, Ilya Kulshin wrote: > On 2016/07/29 23:14:08, jam wrote: > > On 2016/07/29 22:38:29, Ilya Kulshin wrote: > > > On 2016/07/29 22:34:58, jam wrote: > > > > > > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > > > File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > > > content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // Hack > > for > > > > crbug.com/631254: set the sender if we can get one, so that when > > > > On 2016/07/28 22:10:41, Ilya Kulshin wrote: > > > > > On 2016/07/28 22:04:43, jam wrote: > > > > > > nit: saying hack makes it sound like this should be fixed properly. > > using > > > > > > thread-safe sender is a standard pattern. > > > > > > > > > > > > I would just get rid of this comment since it's documenting what the > > code > > > is > > > > > > doing. > > > > > > > > > > This should be fixed properly. One possible fix we are talking about is > to > > > > > detect when the pepper font api is being called from the wrong thread, > and > > > > > marshal the call to the appropriate thread. > > > > > > > > I don't understand this comment. That's what the SafeSender is doing > > > internally? > > > > > > The SafeSender will make sure the IPC runs from the right thread, correct? > > > > Correct> > > > > > The > > > problem is that there's a bunch of non-IPC code that runs before it even > gets > > to > > > the point where it knows it needs to send an IPC, and that code also is not > > > being called from the right thread. > > > > I don't know what code you're talking about. But generally the pattern is on > > startup we save a pointer to safe sender and then call it later on different > > threads. Can't that be done here as well? > > > Consider the callstack in > https://bugs.chromium.org/p/chromium/issues/detail?id=631254#c3. This is > happening during process runtime, and happens to be not on the main thread. At > the top of the stack is DWriteFontCollectionProxy, which actually calls the IPC > code. Before that there is Skia code, and before that there is blink code. > Although the code was crashing because the font proxy was unable to obtain a > sender, the blink code is also triggering DCHECKs because it is being called on > the wrong thread. The blink code is the code that I want to marshal to a > different thread (and therefore also the Skia and font proxy code that blink > will call). That marshaling will need to happen in > content::BrowserFontResource_Trusted::Describe as well as any other similar > functions that are called by the plugin code. > > As far as saving a pointer to the safe sender and then calling it later on > different threads, that is exactly what this patch implements. It is still not > sufficient, because by the time it gets to the point where it needs the sender, > we are already running code on the wrong thread. Why can't some of the blink or pepper code in that callstack have some context that (indirectly or directly) holds onto the safe sender pointer? i.e. InitializeDWriteFontProxy is called before blink code is running.
Message was sent while issue was closed.
On 2016/08/01 22:23:28, jam wrote: > On 2016/07/30 00:25:20, Ilya Kulshin wrote: > > On 2016/07/29 23:14:08, jam wrote: > > > On 2016/07/29 22:38:29, Ilya Kulshin wrote: > > > > On 2016/07/29 22:34:58, jam wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > > > > File content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2182213004/diff/120001/content/child/dwrite_f... > > > > > content/child/dwrite_font_proxy/dwrite_font_proxy_init_win.cc:81: // > Hack > > > for > > > > > crbug.com/631254: set the sender if we can get one, so that when > > > > > On 2016/07/28 22:10:41, Ilya Kulshin wrote: > > > > > > On 2016/07/28 22:04:43, jam wrote: > > > > > > > nit: saying hack makes it sound like this should be fixed properly. > > > using > > > > > > > thread-safe sender is a standard pattern. > > > > > > > > > > > > > > I would just get rid of this comment since it's documenting what the > > > code > > > > is > > > > > > > doing. > > > > > > > > > > > > This should be fixed properly. One possible fix we are talking about > is > > to > > > > > > detect when the pepper font api is being called from the wrong thread, > > and > > > > > > marshal the call to the appropriate thread. > > > > > > > > > > I don't understand this comment. That's what the SafeSender is doing > > > > internally? > > > > > > > > The SafeSender will make sure the IPC runs from the right thread, correct? > > > > > > > Correct> > > > > > > > The > > > > problem is that there's a bunch of non-IPC code that runs before it even > > gets > > > to > > > > the point where it knows it needs to send an IPC, and that code also is > not > > > > being called from the right thread. > > > > > > I don't know what code you're talking about. But generally the pattern is on > > > startup we save a pointer to safe sender and then call it later on different > > > threads. Can't that be done here as well? > > > > > Consider the callstack in > > https://bugs.chromium.org/p/chromium/issues/detail?id=631254#c3. This is > > happening during process runtime, and happens to be not on the main thread. At > > the top of the stack is DWriteFontCollectionProxy, which actually calls the > IPC > > code. Before that there is Skia code, and before that there is blink code. > > Although the code was crashing because the font proxy was unable to obtain a > > sender, the blink code is also triggering DCHECKs because it is being called > on > > the wrong thread. The blink code is the code that I want to marshal to a > > different thread (and therefore also the Skia and font proxy code that blink > > will call). That marshaling will need to happen in > > content::BrowserFontResource_Trusted::Describe as well as any other similar > > functions that are called by the plugin code. > > > > As far as saving a pointer to the safe sender and then calling it later on > > different threads, that is exactly what this patch implements. It is still not > > sufficient, because by the time it gets to the point where it needs the > sender, > > we are already running code on the wrong thread. > > Why can't some of the blink or pepper code in that callstack have some context > that (indirectly or directly) holds onto the safe sender pointer? i.e. > InitializeDWriteFontProxy is called before blink code is running. Blink uses the DirectWrite API to talk to the font proxy, and that doesn't have any way to pass context, other than being object-oriented. I'm also not sure I understand your question, since the lack of context isn't an issue - the IPC sender is saved as a member of the object, so there's no difficulty in retrieving the saved value once inside the font proxy code. |
