764541 - Crash in BidiParagraphData::PushBidiControl

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Reproduces on trunk
20120613124512
http://hg.mozilla.org/mozilla-central/rev/00244ceddd42

Reproduces with one firefox instance on a slow bot, on a faster local machine, you need to have a couple of instances started simultaneously e.g using

import os
thread_num = 15
for i in range(0, thread_num):
  profile_dir = '/tmp/firefox_profile_%d' % i
  os.system('mkdir %s' % profile_dir)
  os.system('touch %s/pref.js' % profile_dir)
  cmd = "path-to-your-firefox/src/objdir-ff-asan/dist/bin/firefox-bin -no-remote -profile '%s' test.html 2>&1 | tee /tmp/%d.log &" % (profile_dir, i)
  os.system(cmd)

ASAN:SIGSEGV
==22606== ERROR: AddressSanitizer crashed on unknown address 0x7f9d5ea7f69e (pc 0x7f9b7d2aa15f sp 0x7fff13209b60 bp 0x7fff13209c90 T0)
AddressSanitizer can not provide additional info. ABORTING
    #0 0x7f9b7d2aa15f in void nsTArrayElementTraits<unsigned short>::Construct<unsigned short>(unsigned short*, unsigned short const&) firefox/src/../../dist/include/nsTArray.h:344
    #1 0x7f9b7d2a9d6e in void nsTArray<unsigned short, nsTArrayDefaultAllocator>::AssignRange<unsigned short>(unsigned int, unsigned int, unsigned short const*) firefox/src/../../dist/include/nsTArray.h:1223
    #2 0x7f9b7d2a968b in unsigned short* nsTArray<unsigned short, nsTArrayDefaultAllocator>::AppendElements<unsigned short>(unsigned short const*, unsigned int) firefox/src/../../dist/include/nsTArray.h:871
    #3 0x7f9b7d2a92ed in unsigned short* nsTArray<unsigned short, nsTArrayDefaultAllocator>::AppendElement<unsigned short>(unsigned short const&) firefox/src/../../dist/include/nsTArray.h:884
    #4 0x7f9b7d26b685 in BidiParagraphData::PushBidiControl(unsigned short) firefox/src/layout/base/nsBidiPresUtils.cpp:267
    #5 0x7f9b7d26cb93 in nsBidiPresUtils::TraverseFrames(nsBlockFrame*, nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) firefox/src/layout/base/nsBidiPresUtils.cpp:959
    #6 0x7f9b7d26eb4d in nsBidiPresUtils::TraverseFrames(nsBlockFrame*, nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) firefox/src/layout/base/nsBidiPresUtils.cpp:1127
    #7 0x7f9b7d26a249 in nsBidiPresUtils::Resolve(nsBlockFrame*) firefox/src/layout/base/nsBidiPresUtils.cpp:598
    #8 0x7f9b7d3f4c49 in nsBlockFrame::ResolveBidi() firefox/src/layout/generic/nsBlockFrame.cpp:7116
    #9 0x7f9b7d3f7e55 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) firefox/src/layout/generic/nsBlockFrame.cpp:1000
    #10 0x7f9b7d48865b in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) firefox/src/layout/generic/nsBlockReflowContext.cpp:262
    #11 0x7f9b7d429947 in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) firefox/src/layout/generic/nsBlockFrame.cpp:3180
    #12 0x7f9b7d41f036 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) firefox/src/layout/generic/nsBlockFrame.cpp:2488
    #13 0x7f9b7d404941 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) firefox/src/layout/generic/nsBlockFrame.cpp:1994
    #14 0x7f9b7d3f83df in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) firefox/src/layout/generic/nsBlockFrame.cpp:1043
    #15 0x7f9b7d4e8967 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) firefox/src/layout/generic/nsContainerFrame.cpp:906
    #16 0x7f9b7d6b7217 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) firefox/src/layout/generic/nsCanvasFrame.cpp:429
    #17 0x7f9b7d4e8967 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) firefox/src/layout/generic/nsContainerFrame.cpp:906
    #18 0x7f9b7d631c4e in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) firefox/src/layout/generic/nsGfxScrollFrame.cpp:516
    #19 0x7f9b7d6374fa in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) firefox/src/layout/generic/nsGfxScrollFrame.cpp:616
    #20 0x7f9b7d63b81f in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) firefox/src/layout/generic/nsGfxScrollFrame.cpp:857
    #21 0x7f9b7d4e8967 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) firefox/src/layout/generic/nsContainerFrame.cpp:906
    #22 0x7f9b7da0da51 in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) firefox/src/layout/generic/nsViewportFrame.cpp:200
    #23 0x7f9b7d176690 in PresShell::DoReflow(nsIFrame*, bool) firefox/src/layout/base/nsPresShell.cpp:7373
    #24 0x7f9b7d1a415d in PresShell::ProcessReflowCommands(bool) firefox/src/layout/base/nsPresShell.cpp:7514
    #25 0x7f9b7d1a286d in PresShell::FlushPendingNotifications(mozFlushType) firefox/src/layout/base/nsPresShell.cpp:3845
    #26 0x7f9b7e9eb95e in nsDocument::FlushPendingNotifications(mozFlushType) firefox/src/content/base/src/nsDocument.cpp:6339
    #27 0x7f9b7ebb1402 in nsGenericElement::GetPrimaryFrame(mozFlushType) firefox/src/content/base/src/nsGenericElement.cpp:3924
    #28 0x7f9b7ebb1046 in nsGenericElement::GetStyledFrame() firefox/src/content/base/src/nsGenericElement.cpp:2010
    #29 0x7f9b7ebb2557 in nsGenericElement::GetScrollFrame(nsIFrame**) firefox/src/content/base/src/nsGenericElement.cpp:2050
    #30 0x7f9b7ebb5ce3 in nsGenericElement::GetClientAreaRect() firefox/src/content/base/src/nsGenericElement.cpp:2199
    #31 0x7f9b7ebb6ed6 in nsGenericElement::GetClientWidth() firefox/src/content/base/src/nsGenericElement.h:562
    #32 0x7f9b83d18bac in nsIDOMElement_GetClientWidth(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*) firefox/src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:4645
    #33 0x7f9b8d31f39e in js::CallJSPropertyOp(JSContext*, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*), JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*) firefox/src/js/src/jscntxtinlines.h:444
    #34 0x7f9b8d2ba749 in js_NativeGetInline(JSContext*, JSObject*, JSObject*, JSObject*, js::Shape const*, unsigned int, JS::Value*) firefox/src/js/src/jsobj.cpp:4931
    #35 0x7f9b8c7fbe9e in JSObject::getGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*) firefox/src/js/src/jsobjinlines.h:162
    #36 0x7f9b8dc2ab55 in js::DirectWrapper::get(JSContext*, JSObject*, JSObject*, long, JS::Value*) firefox/src/js/src/jswrapper.cpp:183
    #37 0x7f9b8d4ec7b4 in js::Proxy::get(JSContext*, JSObject*, JSObject*, long, JS::Value*) firefox/src/js/src/jsproxy.cpp:1090
    #38 0x7f9b8d4fde86 in proxy_GetGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*) firefox/src/js/src/jsproxy.cpp:1303
    #39 0x7f9b8c7fb9f9 in JSObject::getGeneric(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*) firefox/src/js/src/jsobjinlines.h:159
    #40 0x7f9b8c8bb4f2 in JSObject::getGeneric(JSContext*, JS::Handle<long>, JS::Value*) firefox/src/js/src/jsobjinlines.h:177
    #41 0x7f9b8d0a4d14 in js::GetPropertyGenericMaybeCallXML(JSContext*, JSOp, JS::Handle<JSObject*>, JS::Handle<long>, JS::Value*) firefox/src/js/src/jsinterpinlines.h:164
    #42 0x7f9b8d07f0b5 in js::GetPropertyOperation(JSContext*, unsigned char*, JS::Value const&, JS::Value*) firefox/src/js/src/jsinterpinlines.h:227
    #43 0x7f9b8cfa6064 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) firefox/src/js/src/jsinterp.cpp:2333
    #44 0x7f9b8cf331d7 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) firefox/src/js/src/jsinterp.cpp:267
    #45 0x7f9b8d03f2b0 in js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) firefox/src/js/src/jsinterp.cpp:322
    #46 0x7f9b8c9b07d0 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) firefox/src/js/src/jsinterp.h:100
    #47 0x7f9b8d04496b in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) firefox/src/js/src/jsinterp.cpp:354
    #48 0x7f9b8c848c21 in JS_CallFunctionValue firefox/src/js/src/jsapi.cpp:5515
    #49 0x7f9b807f804f in nsXBLProtoImplAnonymousMethod::Execute(nsIContent*) firefox/src/content/xbl/src/nsXBLProtoImplMethod.cpp:333
    #50 0x7f9b8075ba94 in nsXBLPrototypeBinding::BindingAttached(nsIContent*) firefox/src/content/xbl/src/nsXBLPrototypeBinding.cpp:490
    #51 0x7f9b8073752d in nsXBLBinding::ExecuteAttachedHandler() firefox/src/content/xbl/src/nsXBLBinding.cpp:913
    #52 0x7f9b80875bdb in nsBindingManager::ProcessAttachedQueue(unsigned int) firefox/src/content/xbl/src/nsBindingManager.cpp:1010
    #53 0x7f9b7d1a251b in PresShell::FlushPendingNotifications(mozFlushType) firefox/src/layout/base/nsPresShell.cpp:3830
    #54 0x7f9b7d1a0fa7 in PresShell::HandlePostedReflowCallbacks(bool) firefox/src/layout/base/nsPresShell.cpp:3686
    #55 0x7f9b7d177d99 in PresShell::DidDoReflow(bool) firefox/src/layout/base/nsPresShell.cpp:7227
    #56 0x7f9b7d17335a in PresShell::ResizeReflowIgnoreOverride(int, int) firefox/src/layout/base/nsPresShell.cpp:1833
    #57 0x7f9b7d174402 in PresShell::ResizeReflow(int, int) firefox/src/layout/base/nsPresShell.cpp:1764
    #58 0x7f9b809d00ba in nsViewManager::DoSetWindowDimensions(int, int) firefox/src/view/src/nsViewManager.cpp:207
    #59 0x7f9b809d0a5d in nsViewManager::SetWindowDimensions(int, int) firefox/src/view/src/nsViewManager.cpp:227
    #60 0x7f9b7cfc6e31 in DocumentViewerImpl::SetBounds(nsIntRect const&) firefox/src/layout/base/nsDocumentViewer.cpp:1881
    #61 0x7f9b8469ef9c in nsDocShell::SetPositionAndSize(int, int, int, int, bool) firefox/src/docshell/base/nsDocShell.cpp:4771
    #62 0x7f9b8469f657 in non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, bool) firefox/src/modules/zlib/src/inffast.c:0
    #63 0x7f9b7eb30a11 in nsFrameLoader::UpdateBaseWindowPositionAndSize(nsIFrame*) firefox/src/content/base/src/nsFrameLoader.cpp:1706
Stats: 292M malloced (347M for red zones) by 734774 calls
Stats: 102M realloced by 82974 calls
Stats: 162M freed by 398209 calls
Stats: 36M really freed by 115959 calls
Stats: 612M (156748 full pages) mmaped in 153 calls
  mmaps   by size class: 8:475107; 9:57337; 10:24570; 11:71645; 12:5120; 13:3072; 14:6400; 15:384; 16:640; 17:128; 18:192; 19:56; 20:16;
  mallocs by size class: 8:561044; 9:60775; 10:24030; 11:73761; 12:4343; 13:2806; 14:6563; 15:402; 16:620; 17:158; 18:201; 19:56; 20:15;
  frees   by size class: 8:305744; 9:48777; 10:20010; 11:15509; 12:3254; 13:2144; 14:1535; 15:341; 16:528; 17:133; 18:172; 19:50; 20:12;
  rfrees  by size class: 8:98011; 9:8112; 10:3844; 11:4528; 12:467; 13:357; 14:212; 15:101; 16:252; 17:40; 18:24; 19:10; 20:1;
Stats: malloc large: 430 small slow: 4273

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsBidiPresUtils.cpp#272
mEmbeddingStack.Length() is zero in PopBidiControl() so we call
nsTArray::TruncateLength(size_t(-1)) which results in a (safe) OOM abort.

Comment 2

[Mats Palmgren (inactive)]

Attachment: frame tree + stack for assertion

This is a log od TraverseFrames calls leading up to the assertion + a dump of the frame tree and stack.

Comment 3

[Abhishek Arya]

I missed to debug this properly, feel free to remove the security tags.

Comment 4

[Mats Palmgren (inactive)]

You did the right thing - all ASAN crashes should be filed as security-sensitive.

Comment 5

[Mats Palmgren (inactive)]

Attachment: more logging

The red and blue frames are last continuations and thus we pop for them.
I have marked the their respective first-continuation in the same color
but with a grey background -- the bug is that we didn't reach the blue
first-continuation (0x7fffcde90380) since it's in an OverflowList, so
we're a push call short.

Comment 6

[Mats Palmgren (inactive)]

Attachment: fix

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=42345431d1f0

Comment 7

[Mats Palmgren (inactive)]

Abhishek, does that patch fix it for you?

Comment 8

[Abhishek Arya]

(In reply to Mats Palmgren [:mats] from comment #7)
> Abhishek, does that patch fix it for you?

The patch fixes the crash.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 633307 [details] [diff] [review]
fix

Traverse the child frames on the OverflowList too.

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 633307 [details] [diff] [review]
fix

Review of attachment 633307 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsBidiPresUtils.cpp
@@ +607,5 @@
>      nsBlockInFlowLineIterator lineIter(block, block->begin_lines());
>      bpd.mPrevFrame = nsnull;
>      bpd.GetSubParagraph()->mPrevFrame = nsnull;
>      TraverseFrames(aBlockFrame, &lineIter, block->GetFirstPrincipalChild(), &bpd);
> +    // XXX what about overflow lines?

So what about them? do we need to add something like
 |TraverseFrames(aBlockFrame, &lineIter, block->GetFirstChild(kOverflowList), &bpd);|
here?

Comment 11

[Simon Montagu :smontagu]

By the way, if frames in OverflowLists aren't going through bidi resolution, I would expect to see RTL text in those frames displayed backwards. Can we get a test case for that? I don't know myself how to create a test case where particular text will be in an OverflowList.

Comment 12

[Mats Palmgren (inactive)]

(In reply to Simon Montagu from comment #10)
> So what about them? do we need to add something like
>  |TraverseFrames(aBlockFrame, &lineIter,
> block->GetFirstChild(kOverflowList), &bpd);|
> here?

Probably.  Can we do this in a separate bug though?
It's not relevant for the crash fix and I'd like to keep the risk
to a minimum for branches.

(In reply to Simon Montagu from comment #11)
> Can we get a test case for that? I don't know myself how to create
> a test case where particular text will be in an OverflowList.

I guess we could start with adding an assertion.

Also, I filed bug 410857 a long time ago, it occurs to me now that
it might also be important for correct bidi handling?

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 633307 [details] [diff] [review]
fix

Review of attachment 633307 [details] [diff] [review]:
-----------------------------------------------------------------

> Probably.  Can we do this in a separate bug though?
> It's not relevant for the crash fix and I'd like to keep the risk
> to a minimum for branches.

Makes sense.

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5be2e11080

Filed bug 766907 to follow-up on review comment 10.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fb5be2e11080

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to Mats Palmgren [:mats] from comment #12)
> It's not relevant for the crash fix and I'd like to keep the risk
> to a minimum for branches.

Maybe I was over-hasty marking this a DoS, do we need this fix on branches? How bad is the problem and how far back does it go?

Comment 17

[Mats Palmgren (inactive)]

The patch applies with minor adjustments to aurora, beta, release, esr10.
I think it's safe OOM abort on all those branches, nsTArray allocation
was made infallible by default for mozilla-2.0 in bug 610823.
I haven't checked what 1.9.2 does when it fails.

The crash seems very unlikely to be triggered in real content, so it
doesn't seem needed for branches.  There's 1 crash reported in
PushBidiControl in the past 4 weeks at crash-stats.m.o.