795313 - Uninitialised value use in nsFontInflationData::UpdateFontInflationDataWidthFor

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Julian Seward [:jseward]]

Appears on every startup of Fennec, running on Xoom w/ ICS (4.0).

The uninitialised value use is actually reported in two different
places, which increases the chances that this is a legit bug (iow, not
a false positive from V).  Both have the same origin though, so it's
the same underlying problem.  I'll include both reports below for
completeness.

Here's the first use point reported ..

Conditional jump or move depends on uninitialised value(s)
   at 0x2FC72D40: nsFontInflationData::UpdateFontInflationDataWidthFor(nsHTMLReflowState const&) (layout/generic/nsFontInflationData.cpp:65)
   by 0x2FC9038D: nsHTMLReflowState::InitResizeFlags(nsPresContext*, nsIAtom*) (layout/generic/nsHTMLReflowState.cpp:440)
   by 0x2FC91011: nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) (layout/generic/nsHTMLReflowState.cpp:316)
   by 0x2FC91479: nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsIFrame*, nsRenderingContext*, nsSize const&, unsigned int) (layout/generic/nsHTMLReflowState.cpp:78)
   by 0x2FC46449: PresShell::DoReflow(nsIFrame*, bool) (layout/base/nsPresShell.cpp:7334)
   by 0x2FC46A6B: PresShell::ProcessReflowCommands(bool) (layout/base/nsPresShell.cpp:7518)
   by 0x2FC49A4D: PresShell::FlushPendingNotifications(mozFlushType) (layout/base/nsPresShell.cpp:3848)
   by 0x2FC4AEF5: nsRefreshDriver::Notify(nsITimer*) (layout/base/nsRefreshDriver.cpp:398)
   by 0x3054217B: nsTimerImpl::Fire() (xpcom/threads/nsTimerImpl.cpp:476)
   by 0x3054232B: nsTimerEvent::Run() (xpcom/threads/nsTimerImpl.cpp:556)
   by 0x3053FB97: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:612)
   by 0x30515F75: NS_ProcessNextEvent_P(nsIThread*, bool) (ff-opt/xpcom/build/nsThreadUtils.cpp:220)
   by 0x3040E307: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:82)
   by 0x3056AB2D: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:208)
   by 0x3056AB3B: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:201)
   by 0x3056AC3B: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:175)
   by 0x3036FC69: nsBaseAppShell::Run() (widget/xpwidgets/nsBaseAppShell.cpp:163)
   by 0x3025D4C9: nsAppStartup::Run() (toolkit/components/startup/nsAppStartup.cpp:290)
   by 0x2FA91369: XREMain::XRE_mainRun() (toolkit/xre/nsAppRunner.cpp:3782)
   by 0x2FA93821: XREMain::XRE_main(int, char**, nsXREAppData const*) (toolkit/xre/nsAppRunner.cpp:3848)

 Uninitialised value was created by a heap allocation
   at 0x4806920: malloc (coregrind/m_replacemalloc/vg_replace_malloc.c:270)
   by 0x226BE645: moz_xmalloc (memory/mozalloc/mozalloc.cpp:57)
   by 0x2FC72D59: nsFontInflationData::UpdateFontInflationDataWidthFor(nsHTMLReflowState const&) (ff-opt/layout/generic/../../dist/include/mozilla/mozalloc.h:200)
   by 0x2FC9038D: nsHTMLReflowState::InitResizeFlags(nsPresContext*, nsIAtom*) (layout/generic/nsHTMLReflowState.cpp:440)
   by 0x2FC91011: nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) (layout/generic/nsHTMLReflowState.cpp:316)
   by 0x2FC91479: nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsIFrame*, nsRenderingContext*, nsSize const&, unsigned int) (layout/generic/nsHTMLReflowState.cpp:78)
   by 0x2FC46449: PresShell::DoReflow(nsIFrame*, bool) (layout/base/nsPresShell.cpp:7334)
   by 0x2FC46A6B: PresShell::ProcessReflowCommands(bool) (layout/base/nsPresShell.cpp:7518)
   by 0x2FC49A4D: PresShell::FlushPendingNotifications(mozFlushType) (layout/base/nsPresShell.cpp:3848)
   by 0x2FC4AEF5: nsRefreshDriver::Notify(nsITimer*) (layout/base/nsRefreshDriver.cpp:398)
   by 0x3054217B: nsTimerImpl::Fire() (xpcom/threads/nsTimerImpl.cpp:476)
   by 0x3054232B: nsTimerEvent::Run() (xpcom/threads/nsTimerImpl.cpp:556)
   by 0x3053FB97: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:612)
   by 0x30515F75: NS_ProcessNextEvent_P(nsIThread*, bool) (ff-opt/xpcom/build/nsThreadUtils.cpp:220)
   by 0x3040E307: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:82)
   by 0x3056AB2D: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:208)
   by 0x3056AB3B: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:201)
   by 0x3056AC3B: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:175)
   by 0x3036FC69: nsBaseAppShell::Run() (widget/xpwidgets/nsBaseAppShell.cpp:163)
   by 0x3025D4C9: nsAppStartup::Run() (toolkit/components/startup/nsAppStartup.cpp:290)


and here's the second.  Note that the origin (the "was created by" bit) is the same
as above.

Conditional jump or move depends on uninitialised value(s)
   at 0x2FC90390: nsHTMLReflowState::InitResizeFlags(nsPresContext*, nsIAtom*) (layout/generic/nsHTMLReflowState.cpp:440)
   by 0x2FC91011: nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) (layout/generic/nsHTMLReflowState.cpp:316)
   by 0x2FC91479: nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsIFrame*, nsRenderingContext*, nsSize const&, unsigned int) (layout/generic/nsHTMLReflowState.cpp:78)
   by 0x2FC46449: PresShell::DoReflow(nsIFrame*, bool) (layout/base/nsPresShell.cpp:7334)
   by 0x2FC46A6B: PresShell::ProcessReflowCommands(bool) (layout/base/nsPresShell.cpp:7518)
   by 0x2FC49A4D: PresShell::FlushPendingNotifications(mozFlushType) (layout/base/nsPresShell.cpp:3848)
   by 0x2FC4AEF5: nsRefreshDriver::Notify(nsITimer*) (layout/base/nsRefreshDriver.cpp:398)
   by 0x3054217B: nsTimerImpl::Fire() (xpcom/threads/nsTimerImpl.cpp:476)
   by 0x3054232B: nsTimerEvent::Run() (xpcom/threads/nsTimerImpl.cpp:556)
   by 0x3053FB97: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:612)
   by 0x30515F75: NS_ProcessNextEvent_P(nsIThread*, bool) (ff-opt/xpcom/build/nsThreadUtils.cpp:220)
   by 0x3040E307: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:82)
   by 0x3056AB2D: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:208)
   by 0x3056AB3B: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:201)
   by 0x3056AC3B: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:175)
   by 0x3036FC69: nsBaseAppShell::Run() (widget/xpwidgets/nsBaseAppShell.cpp:163)
   by 0x3025D4C9: nsAppStartup::Run() (toolkit/components/startup/nsAppStartup.cpp:290)
   by 0x2FA91369: XREMain::XRE_mainRun() (toolkit/xre/nsAppRunner.cpp:3782)
   by 0x2FA93821: XREMain::XRE_main(int, char**, nsXREAppData const*) (toolkit/xre/nsAppRunner.cpp:3848)
   by 0x2FA93A39: XRE_main (toolkit/xre/nsAppRunner.cpp:3923)

 Uninitialised value was created by a heap allocation
   at 0x4806920: malloc (coregrind/m_replacemalloc/vg_replace_malloc.c:270)
   by 0x226BE645: moz_xmalloc (memory/mozalloc/mozalloc.cpp:57)
   by 0x2FC72D59: nsFontInflationData::UpdateFontInflationDataWidthFor(nsHTMLReflowState const&) (ff-opt/layout/generic/../../dist/include/mozilla/mozalloc.h:200)
   by 0x2FC9038D: nsHTMLReflowState::InitResizeFlags(nsPresContext*, nsIAtom*) (layout/generic/nsHTMLReflowState.cpp:440)
   by 0x2FC91011: nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) (layout/generic/nsHTMLReflowState.cpp:316)
   by 0x2FC91479: nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsIFrame*, nsRenderingContext*, nsSize const&, unsigned int) (layout/generic/nsHTMLReflowState.cpp:78)
   by 0x2FC46449: PresShell::DoReflow(nsIFrame*, bool) (layout/base/nsPresShell.cpp:7334)
   by 0x2FC46A6B: PresShell::ProcessReflowCommands(bool) (layout/base/nsPresShell.cpp:7518)
   by 0x2FC49A4D: PresShell::FlushPendingNotifications(mozFlushType) (layout/base/nsPresShell.cpp:3848)
   by 0x2FC4AEF5: nsRefreshDriver::Notify(nsITimer*) (layout/base/nsRefreshDriver.cpp:398)
   by 0x3054217B: nsTimerImpl::Fire() (xpcom/threads/nsTimerImpl.cpp:476)
   by 0x3054232B: nsTimerEvent::Run() (xpcom/threads/nsTimerImpl.cpp:556)
   by 0x3053FB97: nsThread::ProcessNextEvent(bool, bool*) (xpcom/threads/nsThread.cpp:612)
   by 0x30515F75: NS_ProcessNextEvent_P(nsIThread*, bool) (ff-opt/xpcom/build/nsThreadUtils.cpp:220)
   by 0x3040E307: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (ipc/glue/MessagePump.cpp:82)
   by 0x3056AB2D: MessageLoop::RunInternal() (ipc/chromium/src/base/message_loop.cc:208)
   by 0x3056AB3B: MessageLoop::RunHandler() (ipc/chromium/src/base/message_loop.cc:201)
   by 0x3056AC3B: MessageLoop::Run() (ipc/chromium/src/base/message_loop.cc:175)
   by 0x3036FC69: nsBaseAppShell::Run() (widget/xpwidgets/nsBaseAppShell.cpp:163)
   by 0x3025D4C9: nsAppStartup::Run() (toolkit/components/startup/nsAppStartup.cpp:290)

Comment 1

[Scott Johnson (:jwir3)]

Julian:

Are you running current nightlies? This looks like it could be related to bug 749186...

Comment 2

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #1)
> Julian:
> 
> Are you running current nightlies? This looks like it could be related to
> bug 749186...

Actually, on second thought, probably not. I was looking at nsFontInflationData.h:65, not .cpp.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So mNCAWidth isn't always initialized sensibly.  And I don't think it ought to be, since the point is that in some cases UpdateWidth doesn't bother computing it because we know we're not going to enable inflation.  So I think we should change:

  return oldNCAWidth != data->mNCAWidth ||
         oldInflationEnabled != data->mInflationEnabled;

into:

  if (oldInflationEnabled != data->mInflationEnabled)
    return true;

  return oldInflationEnabled &&
         oldNCAWidth != data->mNCAWidth;


This basically makes two changes:
 (1) it swaps the order of comparison of inflation-enabled and NCA-width
 (2) it only compares NCA-widths when inflation is enabled


I think this could be a real performance problem, actually.

Comment 4

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #3)
> So mNCAWidth isn't always initialized sensibly.  And I don't think it ought
> to be, since the point is that in some cases UpdateWidth doesn't bother
> computing it because we know we're not going to enable inflation.  So I
> think we should change:
> 
>   return oldNCAWidth != data->mNCAWidth ||
>          oldInflationEnabled != data->mInflationEnabled;
> 
> into:
> 
>   if (oldInflationEnabled != data->mInflationEnabled)
>     return true;
> 
>   return oldInflationEnabled &&
>          oldNCAWidth != data->mNCAWidth;
> 
> 
> This basically makes two changes:
>  (1) it swaps the order of comparison of inflation-enabled and NCA-width
>  (2) it only compares NCA-widths when inflation is enabled

Agreed. I think this is a good solution.

> I think this could be a real performance problem, actually.

Do you think this code is currently causing widths to be computed when they are unnecessary? It seems like it would, but only initially, since once the ncaWidth is computed once, it will no longer be different from oldNCAWidth. So, I agree we'd have a performance problem once (and, actually, probably if the width changed somehow...), but do you have an idea of how significant this would be?

Comment 5

[Scott Johnson (:jwir3)]

Attachment: b795313

Patch as specified in comment.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 665931 [details] [diff] [review]
b795313

Yeah, you're right, this might not be a performance problem.

Comment 7

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f62de173456

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8f62de173456