Closed Bug 883727 Opened 7 years ago Closed 7 years ago

Fix -Wsometimes-uninitialized warning in xpwidgets/PuppetWidget.cpp

Categories

(Core :: Widget, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox22 --- unaffected
firefox23 --- affected
firefox24 --- affected
firefox25 --- wontfix
firefox26 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Attached patch fix-xpwidget-warning.patch (obsolete) — Splinter Review
This is the only warning I see in widget/xpwidget code when building Firefox for OSX. This warning is a regression from bug 846881.

widget/xpwidgets/PuppetWidget.cpp:126:9 [-Wsometimes-uninitialized] variable 'chromeSeqno' is used uninitialized whenever 'if' condition is false
Attachment #763390 - Flags: review?(bzbarsky)
Comment on attachment 763390 [details] [diff] [review]
fix-xpwidget-warning.patch

If we ever had !mTabChild here, this would leave the members uninitialized...

How about we MOZ_ASSERT(mTabChild) up front in this method and remove the null-check instead?
Attachment #763390 - Flags: review?(bzbarsky) → review-
Patch v2 replaces `if (mTabChild)` with `MOZ_ASSERT(mTabChild)`. Try run did not hit an assertion failure.

https://tbpl.mozilla.org/?tree=Try&rev=a8179a2f223d
Attachment #763390 - Attachment is obsolete: true
Attachment #798153 - Flags: review?(bzbarsky)
Blocks: buildwarning
Do we have debug tests on try that exercise this code?
Flags: needinfo?(cpeterson)
(In reply to Boris Zbarsky [:bz] from comment #3)
> Do we have debug tests on try that exercise this code?

Good question. I don't know if we have tests for the puppet IME code.

I added some MOZ_CRASH() checks to PuppetWidget::InitIMEState(). When running the try tests, mTabChild is always non-null (as expected), but mNeedIMEStateInit is never true. So my patch replaces `if (mTabChild)` with `MOZ_ASSERT(mTabChild)` without changing the control flow.
Flags: needinfo?(cpeterson)
Comment on attachment 798153 [details] [diff] [review]
fix-xpwidget-warning-v2.patch

Ah, OK.  MOZ_CRASH gives me the answer I wanted; I was worried about us not running any debug tests on try that exercise this code.

r=me
Attachment #798153 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/d818d731bd7e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.