Abhishek Arya Reporter
	Description • 10 years ago

=================================================================
==11727==ERROR: AddressSanitizer: heap-use-after-free on address 0x610000218640 at pc 0x7f9b13cc51aa bp 0x7fff71f282d0 sp 0x7fff71f282c8
READ of size 8 at 0x610000218640 thread T0
    #0 0x7f9b13cc51a9 in mozilla::CustomCounterStyle::IsOrdinalInAutoRange(int) layout/style/CounterStyleManager.cpp:1302:14
    #1 0x7f9b13cc4df4 in mozilla::CustomCounterStyle::IsOrdinalInRange(int) layout/style/CounterStyleManager.cpp:1285:10
    #2 0x7f9b13cbea8b in mozilla::CounterStyle::GetCounterText(int, mozilla::WritingMode, nsAString_internal&, bool&) layout/style/CounterStyleManager.cpp:1697:18
    #3 0x7f9b143d63f8 in nsBulletFrame::GetListItemText(nsAString_internal&) layout/generic/nsBulletFrame.cpp:470:3
    #4 0x7f9b143d7b0b in nsBulletFrame::GetDesiredSize(nsPresContext*, nsRenderingContext*, nsHTMLReflowMetrics&, float) layout/generic/nsBulletFrame.cpp:577:7
    #5 0x7f9b143d9243 in nsBulletFrame::GetMinISize(nsRenderingContext*) layout/generic/nsBulletFrame.cpp:636:3
    #6 0x7f9b142aab97 in nsLayoutUtils::IntrinsicForContainer(nsRenderingContext*, nsIFrame*, nsLayoutUtils::IntrinsicISizeType, unsigned int) layout/base/nsLayoutUtils.cpp:3790:16
    #7 0x7f9b144557b7 in nsFrame::AddInlineMinISize(nsRenderingContext*, nsIFrame::InlineMinISizeData*) layout/generic/nsFrame.cpp:3853:25
    #8 0x7f9b14375838 in nsBlockFrame::GetMinISize(nsRenderingContext*) layout/generic/nsBlockFrame.cpp:734:11
    #9 0x7f9b1445c6d6 in nsFrame::ShrinkWidthToFit(nsRenderingContext*, int) layout/generic/nsFrame.cpp:4287:22
    #10 0x7f9b143fa9d9 in nsContainerFrame::ComputeAutoSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, bool) layout/generic/nsContainerFrame.cpp:906:27
    #11 0x7f9b1445959b in nsFrame::ComputeSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, unsigned int) layout/generic/nsFrame.cpp:4063:24
    #12 0x7f9b143cfa28 in FloatMarginWidth(nsHTMLReflowState const&, int, nsIFrame*, nsCSSOffsetState const&) layout/generic/nsBlockReflowState.cpp:605:10
    #13 0x7f9b143bb4f8 in nsBlockReflowState::FlowAndPlaceFloat(nsIFrame*) layout/generic/nsBlockReflowState.cpp:659:30
    #14 0x7f9b143ceacb in nsBlockReflowState::AddFloat(nsLineLayout*, nsIFrame*, int) layout/generic/nsBlockReflowState.cpp:557:14
    #15 0x7f9b1431ee1e in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) layout/generic/nsLineLayout.h:155:12
    #16 0x7f9b143a7589 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) layout/generic/nsBlockFrame.cpp:3799:3
    #17 0x7f9b143a5958 in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) layout/generic/nsBlockFrame.cpp:3599:5
    #18 0x7f9b1439ad78 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3469:9
    #19 0x7f9b14387e11 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2150:7
    #20 0x7f9b1437af7d in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1136:3
    #21 0x7f9b143a2b58 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:290:3
    #22 0x7f9b143974ec in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3196:5
    #23 0x7f9b14387e11 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2150:7
    #24 0x7f9b1437af7d in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1136:3
    #25 0x7f9b143a2b58 in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) layout/generic/nsBlockReflowContext.cpp:290:3
    #26 0x7f9b143974ec in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) layout/generic/nsBlockFrame.cpp:3196:5
    #27 0x7f9b14387e11 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) layout/generic/nsBlockFrame.cpp:2150:7
    #28 0x7f9b1437af7d in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsBlockFrame.cpp:1136:3
    #29 0x7f9b143e6d19 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:945:3
    #30 0x7f9b143e4f1e in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsCanvasFrame.cpp:567:5
    #31 0x7f9b143e6d19 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:945:3
    #32 0x7f9b144bece3 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) layout/generic/nsGfxScrollFrame.cpp:474:3
    #33 0x7f9b144c3887 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) layout/generic/nsGfxScrollFrame.cpp:583:3
    #34 0x7f9b144c67a7 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsGfxScrollFrame.cpp:817:3
    #35 0x7f9b143e6d19 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) layout/generic/nsContainerFrame.cpp:945:3
    #36 0x7f9b146b4e38 in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) layout/generic/nsViewportFrame.cpp:216:7
    #37 0x7f9b13fe5630 in PresShell::DoReflow(nsIFrame*, bool) layout/base/nsPresShell.cpp:8833:3
    #38 0x7f9b140078b7 in PresShell::ProcessReflowCommands(bool) layout/base/nsPresShell.cpp:8993:24
    #39 0x7f9b140055e6 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4265:11
    #40 0x7f9b1406f18c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1284:11
    #41 0x7f9b1407ebf6 in mozilla::InactiveRefreshDriverTimer::TickOne() layout/base/nsRefreshDriver.cpp:173:5
    #42 0x7f9b0d8a580d in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:618:7
    #43 0x7f9b0d8a68fb in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:711:3
    #44 0x7f9b0d895c8a in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:823:7
    #45 0x7f9b0d909f12 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #46 0x7f9b0e533c84 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:99:21
    #47 0x7f9b0e4cbef0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:229:3
    #48 0x7f9b1263de72 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:164:3
    #49 0x7f9b15d995a2 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:280:19
    #50 0x7f9b15ed2409 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4108:10
    #51 0x7f9b15ed3522 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4179:8
    #52 0x7f9b15ed44bc in XRE_main toolkit/xre/nsAppRunner.cpp:4393:16
    #53 0x4bdf1d in main browser/app/nsBrowserApp.cpp:282:12
    #54 0x7f9b22406de4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260
0x610000218640 is located 0 bytes inside of 192-byte region [0x610000218640,0x610000218700)
freed by thread T0 here:
    #0 0x49cee1 in __interceptor_free _asan_rtl_
    #1 0x7f9b13d3787c in mozilla::CustomCounterStyle::Release() objdir-ff-asan/dist/include/mozilla/mozalloc.h:225:12
    #2 0x7f9b13d40064 in nsTArray_Impl<nsRefPtr<mozilla::CounterStyle>, nsTArrayInfallibleAllocator>::RemoveElementsAt(unsigned long, unsigned long) objdir-ff-asan/dist/include/nsAutoPtr.h:852:7
    #3 0x7f9b13cccc5f in mozilla::CounterStyleManager::NotifyRuleChanged() objdir-ff-asan/dist/include/nsTArray.h:1378:18
    #4 0x7f9b142ede43 in nsPresContext::FlushCounterStyles() layout/base/nsPresContext.cpp:2203:20
    #5 0x7f9b14300408 in nsRunnableMethodImpl<void (nsPresContext::*)(), void, true>::Run() objdir-ff-asan/dist/include/nsThreadUtils.h:388:7
    #6 0x7f9b0d895c8a in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:823:7
    #7 0x7f9b0d909f12 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:265:10
    #8 0x7f9b0e533c84 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:99:21
    #9 0x7f9b0e4cbef0 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:229:3
    #10 0x7f9b1263de72 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:164:3
    #11 0x7f9b15d995a2 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:280:19
    #12 0x7f9b15ed2409 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4108:10
    #13 0x7f9b15ed3522 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4179:8
    #14 0x7f9b15ed44bc in XRE_main toolkit/xre/nsAppRunner.cpp:4393:16
    #15 0x4bdf1d in main browser/app/nsBrowserApp.cpp:282:12
    #16 0x7f9b22406de4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260

previously allocated by thread T0 here:
    #0 0x49d1b9 in malloc _asan_rtl_
    #1 0x7f9b1d4469dd in moz_xmalloc memory/mozalloc/mozalloc.cpp:52:17
    #2 0x7f9b13cc12b6 in mozilla::CounterStyleManager::BuildCounterStyle(nsAString_internal const&) objdir-ff-asan/dist/include/mozilla/mozalloc.h:201:12
    #3 0x7f9b13ccb581 in mozilla::CustomCounterStyle::ComputeExtends() layout/style/CounterStyleManager.cpp:1600:31
    #4 0x7f9b13cc2cc1 in mozilla::CustomCounterStyle::GetExtends() layout/style/CounterStyleManager.cpp:1630:5
    #5 0x7f9b13cc3ad0 in mozilla::CustomCounterStyle::GetExtendsRoot() layout/style/CounterStyleManager.cpp:1639:30
    #6 0x7f9b13cc3915 in mozilla::CustomCounterStyle::IsBullet() layout/style/CounterStyleManager.cpp:1220:14
    #7 0x7f9b143c6b73 in nsBlockFrame::SetInitialChildList(mozilla::layout::FrameChildListID, nsFrameList&) layout/generic/nsBlockFrame.cpp:6540:46
    #8 0x7f9b1416293e in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) layout/base/nsCSSFrameConstructor.cpp:10779:3
    #9 0x7f9b14173e02 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) layout/base/nsCSSFrameConstructor.cpp:4722:3
    #10 0x7f9b1416e1e0 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) layout/base/nsCSSFrameConstructor.cpp:3740:7
    #11 0x7f9b1417ac4d in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) layout/base/nsCSSFrameConstructor.cpp:5803:3
    #12 0x7f9b141580de in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) layout/base/nsCSSFrameConstructor.cpp:9565:5
    #13 0x7f9b141628a5 in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) layout/base/nsCSSFrameConstructor.cpp:10775:3
    #14 0x7f9b14173e02 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) layout/base/nsCSSFrameConstructor.cpp:4722:3
    #15 0x7f9b1416e1e0 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) layout/base/nsCSSFrameConstructor.cpp:3740:7
    #16 0x7f9b1417ac4d in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) layout/base/nsCSSFrameConstructor.cpp:5803:3
    #17 0x7f9b1418b4e5 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) layout/base/nsCSSFrameConstructor.cpp:9565:5
    #18 0x7f9b14185072 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) layout/base/nsCSSFrameConstructor.cpp:6618:5
    #19 0x7f9b1418511c in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) layout/base/nsCSSFrameConstructor.cpp:6625:7
    #20 0x7f9b1418c554 in nsCSSFrameConstructor::CreateNeededFrames() layout/base/nsCSSFrameConstructor.cpp:6640:5
    #21 0x7f9b140e354b in mozilla::RestyleManager::ProcessPendingRestyles() layout/base/RestyleManager.cpp:1494:3
    #22 0x7f9b14004db4 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4224:9
    #23 0x7f9b128265f7 in nsDocument::FlushPendingNotifications(mozFlushType) content/base/src/nsDocument.cpp:7938:7
    #24 0x7f9b0f67535e in nsDocLoader::DocLoaderIsEmpty(bool) uriloader/base/nsDocLoader.cpp:694:9
    #25 0x7f9b0f678b66 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) uriloader/base/nsDocLoader.cpp:624:5
    #26 0x7f9b0f679bdc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) uriloader/base/nsDocLoader.cpp:628:1
    #27 0x7f9b0db0bc26 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) netwerk/base/src/nsLoadGroup.cpp:689:18
    #28 0x7f9b12832a9b in nsDocument::DoUnblockOnload() content/base/src/nsDocument.cpp:8834:7
    #29 0x7f9b128321d5 in nsDocument::UnblockOnload(bool) content/base/src/nsDocument.cpp:8762:9

SUMMARY: AddressSanitizer: heap-use-after-free ??:0 ??
Shadow bytes around the buggy address:
  0x0c208003b070: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c208003b080: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c208003b090: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c208003b0a0: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
  0x0c208003b0b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
=>0x0c208003b0c0: fa fa fa fa fa fa fa fa[fd]fd fd fd fd fd fd fd
  0x0c208003b0d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c208003b0e0: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c208003b0f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c208003b100: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
  0x0c208003b110: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 03 fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  ASan internal:           fe
==11727==ABORTING

	Daniel Holbert [:dholbert] Assignee
	Comment 1 • 10 years ago

I can reproduce, using attached testcase and an ASAN-enabled debug build.

(FWIW: I don't get any assertion-failures or any other scary-looking output before the ASAN abort.)

	Daniel Holbert [:dholbert] Assignee
	Comment 3 • 10 years ago

So, we're aborting when we try to call a method on mExtendsRoot (via GetExtendsRoot()) here:
> 1288 /* virtual */ bool
> 1289 CustomCounterStyle::IsOrdinalInAutoRange(CounterValue aOrdinal)
> 1290 {
[...]
> 1301     case NS_STYLE_COUNTER_SYSTEM_EXTENDS:
> 1302       return GetExtendsRoot()->IsOrdinalInAutoRange(aOrdinal);
http://mxr.mozilla.org/mozilla-central/source/layout/style/CounterStyleManager.cpp?rev=9d4e083655b9#1302

mExtends has type CounterStyle* (raw pointer), but it points to a refcounted object. And the refcounted object that it points to appears to have been destructed.

So, we're calling a virtual method (IsOrdinalInAutoRange) on a deleted value. Hence, marking sec-critical.

I don't know how the lifetime management here is supposed to work, but maybe mExtends needs to be a nsRefPtr...  (Or if that causes a cycle, and there's some logic that's supposed to ensure that it never points to something dead, then that logic apparently isn't working right.)

	Daniel Holbert [:dholbert] Assignee
	Comment 4 • 10 years ago

(er, meant to say "mExtendsRoot" instead of "mExtends" in previous comment; though it's possible that mExtends could have similar issues. In this case, mExtends and mExtendsRoot both point to the same deleted object.)

	Daniel Holbert [:dholbert] Assignee
	Comment 5 • 10 years ago

[Tracking Requested - why for this release]:

Looks like this file was created in bug 966166 (in http://hg.mozilla.org/mozilla-central/rev/001216bdc605 ), which suggests that this bug has been around since that landed, which means this likely affects Firefox 33 (currently on Beta).  Hence, requesting tracking.  Worth checking whether this is reproducible in an ASAN beta build, but lacking that, we should assume Beta33 is affected and try to get this fixed there before it goes to release.

	Daniel Holbert [:dholbert] Assignee
	Comment 6 • 10 years ago

So it looks like we get a call to "InvalidateOldStyle" for each of our CustomCounterStyleRule instances, and in each case we set "toBeRemoved = true" and remove it from mCacheTable.

But we end up keeping the same value in our bullet frame's StyleList()->mCounterStyle variable.  *That* variable is a nsRefPtr, so it sticks around, even though it's been removed from mCacheTable, and its mExtends & mExtendsRoot variables point to a now-dead object.

So, after the restyle, we end up using the unchanged mCounterStyle (with its unchanged pointer to mExtendsRoot) and referencing deleted memory.

	Daniel Holbert [:dholbert] Assignee
	Comment 7 • 10 years ago

Here's a further-reduced testcase.

This one does not abort, but it renders incorrectly (without the correct "b" character in the bullet-point).  As noted in a comment inside the testcase, the problem goes away if you remove a seemingly-trivial "ul {}" rule from the added style.

I suspect this misrendering is a symptom of the same underlying problem.

	Daniel Holbert [:dholbert] Assignee
	Comment 8 • 10 years ago

(Also, FWIW, both the original testcase and testcase 2 crash my Nightly build -- no ASAN needed.)

	Daniel Holbert [:dholbert] Assignee
	Comment 9 • 10 years ago

Here's a bug with testcase 2 (reduced) as a crashtest, for usage whenever this bug is fixed everywhere & ready to be opened up.

	Daniel Holbert [:dholbert] Assignee
	Comment 10 • 10 years ago

This patch fixes the crash by making us null out the CustomCounterStyle's pointers to other CounterStyle objects (in particular, to the one that's about to get deleted), when we think we're done with it.

(As it turns out, we're not actually done with it, because it still exists on the style struct; hence, we need to make sure it doesn't have any pointers to old styles.)

This fixes the crash on testcases 1 and 2, but does not fix testcase 3.  (We might want to fix testcase 3 in a public bug, and it might be better for Xidorn to look at that one, if he has time to take it.)

	Daniel Holbert [:dholbert] Assignee
	Comment 11 • 10 years ago

Here's the documentation for ResetDependentData() (the function I'm using), FWIW:
 http://mxr.mozilla.org/mozilla-central/source/layout/style/CounterStyleManager.cpp#1006

	Xidorn Quan [:xidorn] UTC+11
	Comment 12 • 10 years ago

Comment on attachment 8488323 [details] [diff] [review]
fix v1: Drop dependent data when we expect to be removing a CustomCounterStyle

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

LGTM. Sorry for this bug. I'll look into testcase 3.

	Xidorn Quan [:xidorn] UTC+11
	Comment 13 • 10 years ago

Comment on attachment 8488323 [details] [diff] [review]
fix v1: Drop dependent data when we expect to be removing a CustomCounterStyle

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

::: layout/style/CounterStyleManager.cpp
@@ +1905,5 @@
>        CustomCounterStyle* style =
>          static_cast<CustomCounterStyle*>(aStyle.get());
>        if (style->GetRule() != newRule) {
>          toBeRemoved = true;
> +        style->ResetDependentData();

Since the documentation says the method should be called when other styles are changed, I think it might be good to add a comment here to state that ResetDependentData() will never be called for styles removed here, which is the reason it is called here.

	Daniel Holbert [:dholbert] Assignee
	Comment 14 • 10 years ago

Despite my suspicion in comment 5, this may not affect comment 33 after all -- I can't make any of the testcases crash in current Beta or Aurora releases, and testcase 3 renders correctly in those builds as well.

So this seems to be a recent Nightly regression from some other style system patch.  Bug 931668 comes to mind as one possible culprit.

	Daniel Holbert [:dholbert] Assignee
	Comment 15 • 10 years ago

(er, "may not affect Firefox 33")

	Xidorn Quan [:xidorn] UTC+11
	Comment 16 • 10 years ago

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Despite my suspicion in comment 5, this may not affect comment 33 after all
> -- I can't make any of the testcases crash in current Beta or Aurora
> releases, and testcase 3 renders correctly in those builds as well.
> 
> So this seems to be a recent Nightly regression from some other style system
> patch.  Bug 931668 comes to mind as one possible culprit.

I think it is still good to apply attachment 8488323 [details] [diff] [review] as it indeed improves the robustness.

It seems that list style data is not invalidated in some case after @counter-style changed.

	Daniel Holbert [:dholbert] Assignee
	Comment 17 • 10 years ago

Yup, I agree that the attached patch would be good for defense in depth. Thanks for the feedback! I'll post a new version with a comment along the lines of your suggestion.

Also: looks like this is indeed a regression from Bug 931668 -- I get this regression range: 
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d3587c227172&tochange=9fd98385062e

Before that bug landed, I'll bet we rebuilt this style struct from scratch (and freed the last reference to the extending custom counter-style), but now we don't rebuild the style struct, so the extending counter-style stays alive, even though it's been removed from the cache (and its extendsRoot has been removed from the cache & deleted).

	Daniel Holbert [:dholbert] Assignee
	Comment 18 • 10 years ago

Added a comment -- does this sound OK, Xidorn?

	Cameron McCormack (:heycam)
	Comment 19 • 10 years ago

Where do we kick off the restyling when we detect a @counter-style rule change?

	Daniel Holbert [:dholbert] Assignee
	Comment 20 • 10 years ago

(now with the comment actually qref'ed into the patch...)

	Xidorn Quan [:xidorn] UTC+11
	Comment 21 • 10 years ago

Comment on attachment 8488370 [details] [diff] [review]
8488368: fix for crash, v2 (now with comment): Drop dependent data when we expect to be removing a CustomCounterStyle

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

It looks good, thanks.

	Cameron McCormack (:heycam)
	Comment 22 • 10 years ago

(In reply to Cameron McCormack (:heycam) from comment #19)
> Where do we kick off the restyling when we detect a @counter-style rule
> change?

Oh, I think it is just due to the style sheet update (so from nsStyleLinkElement::DoUpdateStyleSheet).

	Daniel Holbert [:dholbert] Assignee
	Comment 23 • 10 years ago

The CounterStyleManager-clearing stuff that's involved here is kicked off from nsIPresShell::ReconstructStyleDataInternal()'s call to mPresContext->RebuildCounterStyles(), FWIW.

(And we call ReconstructStyleDataInternal, by way of ReconstructStyleData, from PresShell::EndUpdate())

	Cameron McCormack (:heycam)
	Comment 24 • 10 years ago

OK, cool.  Sounds like your comment 6 analysis is probably correct then: the restyleManager->PostRestyleEvent(..., eRestyle_Subtree, ...) call in nsIPresShell::ReconstructStyleDataInternal might not end up restyling the entire tree, keeping the old pointers to CounterStyle objects.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 25 • 10 years ago

Adjusting assignee field to reflect my perceived reality.

	Daniel Holbert [:dholbert] Assignee
	Comment 26 • 10 years ago

Thanks!

sec-approval isn't needed, since this only affects Nightly (per comment 17), so I'm going to go ahead and land.

Once we've shipped a Nightly with this fixed, I'll file a non-security-sensitive followup on testcase 3's misrendering, and maybe Xidorn can take a look at that. (I can take a look at it, too, but I'd need to dive a bit more into this code first to see how these counter styles are managed.)

	Daniel Holbert [:dholbert] Assignee
	Comment 27 • 10 years ago

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5b194ba44e

needinfo=me to file a followup

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/5e5b194ba44e

	Daniel Holbert [:dholbert] Assignee
	Comment 29 • 10 years ago

(bug 1069716 is the followup for testcase 3)

	Daniel Holbert [:dholbert] Assignee
	Comment 30 • 10 years ago

I landed the crashtest and am un-hiding this bug, since this has been fixed in Nightly for ~5 days now:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/447459c9abd1

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/447459c9abd1

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 32 • 10 years ago

Confirmed crash on Fx35, 2014-09-15.
Verified fixed on Fx35 release candidate, 2014-01-06.