841477 - Enable FAIL_ON_WARNINGS on MSVC in dom/plugins/

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Masatoshi Kimura [:emk]]

Attachment: patch

Comment 1

[Benjamin Smedberg]

Comment on attachment 714022 [details] [diff] [review]
patch

What's up with the -EHsc in testplugin.mk ?

Comment 2

[Masatoshi Kimura [:emk]]

MSVC complained with "warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc".

Comment 3

[Benjamin Smedberg]

Where? The C++ exception handler should not be on for the testplugin.

Comment 4

[Masatoshi Kimura [:emk]]

It is not in our code, but in MSVC's stl header.

 0:11.19 C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE\xlocale(
323) : error C2220: 警告をエラーとして扱いました。'object' ファイルは生成されま
せん。
 0:11.20
 0:11.20 Warning: C4530 in C:\Program Files (x86)\Microsoft Visual Studio 10.0\V
C\INCLUDE\xlocale: C++ 例外処理を使っていますが、アンワインド セマンティクスは有
効にはなりません。/EHsc を指定してください。
 0:11.20 C:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\INCLUDE\xlocale(
323) : warning C4530: C++ 例外処理を使っていますが、アンワインド セマンティクス
は有効にはなりません。/EHsc を指定してください。

We are disabling the warning by using the stl wrapper in our main code base, but testplugin is compiled without the stl wrapper:
> # Don't use STL wrappers; nptest isn't Gecko code
> STL_FLAGS =

Comment 5

[Benjamin Smedberg]

Comment on attachment 714022 [details] [diff] [review]
patch

Then we should not be enabling fail-on-warnings for that directory, or we should figure out another way to suppress that error.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Enable FAIL_ON_WARNINGS on MSVC in dom/plugins

OK, excluding testplugin/ for now.

Comment 7

[Masatoshi Kimura [:emk]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a33d38e3a1a

Comment 8

[Daniel Holbert [:dholbert]]

Apparently this is causing local build bustage (bsmith mentioned in IRC, at least):

<bsmith> unused local variable warning in dom/plugins/ipc/PluginModuleParent.cpp treated as error (base::ProcessHandle handle;)

I suspect he's referring to this handle at line 460:
456 #ifdef XP_WIN
457     // collect cpu usage for plugin processes
458 
459     InfallibleTArray<base::ProcessHandle> processHandles;
460     base::ProcessHandle handle;
461 
462     processHandles.AppendElement(OtherProcess());
463 #ifdef MOZ_CRASHREPORTER_INJECTOR
464     if (mFlashProcess1 && base::OpenProcessHandle(mFlashProcess1, &handle)) {
465       processHandles.AppendElement(handle);
466     }
467     if (mFlashProcess2 && base::OpenProcessHandle(mFlashProcess2, &handle)) {
468       processHandles.AppendElement(handle);
469     }
470 #endif
471 
472     if (!GetProcessCpuUsage(processHandles, mPluginCpuUsageOnHang)) {
473       mPluginCpuUsageOnHang.Clear();
474     }
475 #endif
https://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleParent.cpp#456

"handle" is declared outside of that MOZ_CRASHREPORTER_INJECTOR block, but its only usages are inside the block.

We should just declare "handle" inside that inner #ifdef block.  emk, mind pushing a followup to do that, with rs=dholbert  (rubber-stamp)?

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Yes, that is the bug I found. I am already working on the patch. I will push it to inbound with rs=dholbert.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #9)
> Yes, that is the bug I found. I am already working on the patch. I will push
> it to inbound with rs=dholbert.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ffa0bcf919a

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/2a33d38e3a1a
https://hg.mozilla.org/mozilla-central/rev/6ffa0bcf919a