Closed Bug 841477 Opened 9 years ago Closed 9 years ago

Enable FAIL_ON_WARNINGS on MSVC in dom/plugins/

Categories

(Core :: Plug-ins, defect)

All
Windows 8
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: emk, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #714022 - Flags: review?(benjamin)
Comment on attachment 714022 [details] [diff] [review]
patch

What's up with the -EHsc in testplugin.mk ?
Flags: needinfo?(VYV03354)
MSVC complained with "warning C4530: C++ exception handler used, but unwind semantics are not enabled. Specify /EHsc".
Flags: needinfo?(VYV03354)
Where? The C++ exception handler should not be on for the testplugin.
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 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.
Attachment #714022 - Flags: review?(benjamin) → review-
OK, excluding testplugin/ for now.
Attachment #714022 - Attachment is obsolete: true
Attachment #718927 - Flags: review?(benjamin)
Attachment #718927 - Flags: review?(benjamin) → review+
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)?
Yes, that is the bug I found. I am already working on the patch. I will push it to inbound with rs=dholbert.
(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
https://hg.mozilla.org/mozilla-central/rev/2a33d38e3a1a
https://hg.mozilla.org/mozilla-central/rev/6ffa0bcf919a
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.