Closed Bug 795313 Opened 7 years ago Closed 7 years ago

Uninitialised value use in nsFontInflationData::UpdateFontInflationDataWidthFor

Categories

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

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jseward, Assigned: jwir3)

Details

(Keywords: valgrind)

Attachments

(1 file)

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)
Julian:

Are you running current nightlies? This looks like it could be related to bug 749186...
(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.
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.
(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?
Attached patch b795313Splinter Review
Patch as specified in comment.
Attachment #665931 - Flags: review?(dbaron)
Comment on attachment 665931 [details] [diff] [review]
b795313

Yeah, you're right, this might not be a performance problem.
Attachment #665931 - Flags: review?(dbaron) → review+
Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f62de173456
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/8f62de173456
Assignee: nobody → sjohnson
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 797002
Product: Core → Core Graveyard
Component: Layout: HTML Frames → Layout: Images
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.