Fix the crash for the AudioShared.

Non-SFI NaCl: Fix the crash for the AudioShared.

It turned out that even for the non-SFI mode, we still need
to use PPB_AudioShared::SetThreadFunctions, although the
underlying function is pthread_create for both.
Otherwise the TLS in IRT for the created thread is not
initialized correctly.

BUG=359710
TEST=Run trybots.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265663

[Mark Seaborn, 2014-03-20 23:23:30]

https://codereview.chromium.org/200283002/diff/1/ppapi/shared_impl/ppb_audio_...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/200283002/diff/1/ppapi/shared_impl/ppb_audio_...
ppapi/shared_impl/ppb_audio_shared.cc:139: if (thread_functions_set) {
PPB_Audio is meant to fail under NaCl (for both SFI and non-SFI modes) if user
code didn't call ppapi_register_thread_creator().  Currently the code you have
here won't fail in this case because it falls back to the trusted plugin code
path (for --register-pepper-plugins).

I think you need a runtime flag for this file to indicate whether the plugin is
a nexe or a trusted plugin.

[hidehiko, 2014-04-21 14:47:39]

Mark, Bill, could you take a look?

Thanks,
- hidehiko

https://codereview.chromium.org/200283002/diff/1/ppapi/shared_impl/ppb_audio_...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/200283002/diff/1/ppapi/shared_impl/ppb_audio_...
ppapi/shared_impl/ppb_audio_shared.cc:139: if (thread_functions_set) {
On 2014/03/20 23:23:30, Mark Seaborn wrote:
> PPB_Audio is meant to fail under NaCl (for both SFI and non-SFI modes) if user
> code didn't call ppapi_register_thread_creator().  Currently the code you have
> here won't fail in this case because it falls back to the trusted plugin code
> path (for --register-pepper-plugins).
> 
> I think you need a runtime flag for this file to indicate whether the plugin
is
> a nexe or a trusted plugin.

Done.

[bbudge, 2014-04-21 17:01:51]

https://codereview.chromium.org/200283002/diff/60001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/200283002/diff/60001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.h:89: static void SetNaClMode(bool value);
Would it ever make sense for this to be called with value == 'false'?
If not, then perhaps we can eliminate the parameter.

[hidehiko, 2014-04-21 17:25:56]

Thank you for review. Could you take another look?

https://codereview.chromium.org/200283002/diff/60001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/200283002/diff/60001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.h:89: static void SetNaClMode(bool value);
On 2014/04/21 17:01:51, bbudge wrote:
> Would it ever make sense for this to be called with value == 'false'?
> If not, then perhaps we can eliminate the parameter.

I see. Done.

[bbudge, 2014-04-21 20:20:29]

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:144: DCHECK_EQ(result, 0);
nit: reverse param order to match below.

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:157: // See comment above about why we
unlock here.
Since this moved relative to the referred comment, change 'above' to 'below'.

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.h:122: bool thread_active_;
Suggestion: rename these nacl_thread_id_ and nacl_thread_active_ to make the
code a little easier to follow. I have difficulty separating use of
audio_thread_ and these two NaCl specific members.

[hidehiko, 2014-04-21 22:59:47]

Could you take another look?

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:144: DCHECK_EQ(result, 0);
On 2014/04/21 20:20:29, bbudge wrote:
> nit: reverse param order to match below.

Done.

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.cc:157: // See comment above about why we
unlock here.
On 2014/04/21 20:20:29, bbudge wrote:
> Since this moved relative to the referred comment, change 'above' to 'below'.

Instead, swapped the comment, as the comment looks equally applicable for both
code. WDYT?

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/200283002/diff/80001/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.h:122: bool thread_active_;
On 2014/04/21 20:20:29, bbudge wrote:
> Suggestion: rename these nacl_thread_id_ and nacl_thread_active_ to make the
> code a little easier to follow. I have difficulty separating use of
> audio_thread_ and these two NaCl specific members.

Done.

[bbudge, 2014-04-21 23:12:24]

https://codereview.chromium.org/200283002/diff/60002/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/200283002/diff/60002/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.h:87: // If set, SetThreadFunctions() must be
called before this class is used.
This comment suggests to me that you could eliminate SetNaClMode. I don't think
that's the case.

Is it really true that SetThreadFunctions must be called before using the class,
or only that it must be called before calling SetStartPlaybackState? Could we
refine the comment to say that?

// Configures this instance to run in a NaCl plugin.
// If called, SetThreadFunctions() must be called before calling
SetStartPlaybackState
// on any instance of this class.

[hidehiko, 2014-04-21 23:32:09]

Could you take another look?

https://codereview.chromium.org/200283002/diff/60002/ppapi/shared_impl/ppb_au...
File ppapi/shared_impl/ppb_audio_shared.h (right):

https://codereview.chromium.org/200283002/diff/60002/ppapi/shared_impl/ppb_au...
ppapi/shared_impl/ppb_audio_shared.h:87: // If set, SetThreadFunctions() must be
called before this class is used.
I see. Done.

On 2014/04/21 23:12:24, bbudge wrote:
> This comment suggests to me that you could eliminate SetNaClMode. I don't
think
> that's the case.
> 
> Is it really true that SetThreadFunctions must be called before using the
class,
> or only that it must be called before calling SetStartPlaybackState? Could we
> refine the comment to say that?
> 
> // Configures this instance to run in a NaCl plugin.
> // If called, SetThreadFunctions() must be called before calling
> SetStartPlaybackState
> // on any instance of this class.

[bbudge, 2014-04-21 23:51:02]

lgtm

[Mark Seaborn, 2014-04-23 02:28:39]

LGTM, thanks

https://codereview.chromium.org/200283002/diff/110001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/200283002/diff/110001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: // TODO(hidehiko): Enable
AudioThreadCreator testing for NonSfi when
You can remove the TODO now.

https://codereview.chromium.org/200283002/diff/110001/ppapi/shared_impl/ppb_a...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/200283002/diff/110001/ppapi/shared_impl/ppb_a...
ppapi/shared_impl/ppb_audio_shared.cc:158: // In general, the audio thread
should not do Pepper calls, but it
Since this comment applies to both g_nacl_mode and !g_nacl_mode, should it
perhaps go before "if (g_nacl_mode)"?

[hidehiko, 2014-04-23 13:32:59]

Thank you for review, Mark, Bill.

Sending to CQ.

https://codereview.chromium.org/200283002/diff/110001/chrome/test/ppapi/ppapi...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/200283002/diff/110001/chrome/test/ppapi/ppapi...
chrome/test/ppapi/ppapi_browsertest.cc:1199: // TODO(hidehiko): Enable
AudioThreadCreator testing for NonSfi when
On 2014/04/23 02:28:40, Mark Seaborn wrote:
> You can remove the TODO now.

Done.

https://codereview.chromium.org/200283002/diff/110001/ppapi/shared_impl/ppb_a...
File ppapi/shared_impl/ppb_audio_shared.cc (right):

https://codereview.chromium.org/200283002/diff/110001/ppapi/shared_impl/ppb_a...
ppapi/shared_impl/ppb_audio_shared.cc:158: // In general, the audio thread
should not do Pepper calls, but it
On 2014/04/23 02:28:40, Mark Seaborn wrote:
> Since this comment applies to both g_nacl_mode and !g_nacl_mode, should it
> perhaps go before "if (g_nacl_mode)"?

Done.

[hidehiko, 2014-04-23 13:33:03]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-04-23 13:33:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/200283002/150001

[commit-bot: I haz the power, 2014-04-23 14:18:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-23 14:18:01]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[hidehiko, 2014-04-23 14:24:10]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-04-23 14:24:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/200283002/150001

[commit-bot: I haz the power, 2014-04-23 16:11:42]

Change committed as 265663

[Nico, 2014-06-17 20:09:23]

When linking nacl_helper, I now get

/usr/bin/ld: obj/components/../ppapi/nacl_irt/nacl_linux.plugin_main.o:
undefined reference to symbol
'_ZN5ppapi16PPB_Audio_Shared18SetThreadFunctionsEPK18PP_ThreadFunctions'
/usr/bin/ld: note:
'_ZN5ppapi16PPB_Audio_Shared18SetThreadFunctionsEPK18PP_ThreadFunctions' is
defined in DSO lib/libppapi_shared.so so try adding it to the linker command
line
lib/libppapi_shared.so: could not read symbols: Invalid operation

That error looks correct: plugin_main.cc calls SetThreadFunctions() and is in
target linux_main which doesn't depend on ppapi_shared. ppapi_shared contains
the cc file defining SetThreadFunctions().

However, ppapi_shared is defined in a file called ppapi_internal.gyp, so I'm not
sure if just adding a dependency on it is the right thing. Can you advise?

[Nico, 2014-06-17 20:09:55]

…nevermind, this mind be some kind of user error. Ignore me for now while I look
a bit more.