Reproduces on trunk 20120825191419 http://hg.mozilla.org/mozilla-central/rev/e874475efe15 ================================================================= ==30534== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f5f56a1dc9d at pc 0x7f5f78caed33 bp 0x7fffd1793670 sp 0x7fffd1793668 READ of size 1 at 0x7f5f56a1dc9d thread T0 #0 0x7f5f78caed32 in nsTextFragment::CharAt(int) const src/layout/base/../../content/base/src/nsTextFragment.h:178 #1 0x7f5f7ddfe7cf in nsHTMLEditor::IsPrevCharInNodeWhitespace(nsIContent*, int, bool*, bool*, nsIContent**, int*) src/editor/libeditor/html/nsHTMLEditor.cpp:901 #2 0x7f5f7dfe2654 in nsHTMLEditRules::GetPromotedPoint(nsHTMLEditRules::RulesEndpoint, nsIDOMNode*, int, EditAction, nsCOMPtr<nsIDOMNode>*, int*) src/editor/libeditor/html/nsHTMLEditRules.cpp:5240 #3 0x7f5f7def0402 in nsHTMLEditRules::PromoteRange(nsIDOMRange*, EditAction) src/editor/libeditor/html/nsHTMLEditRules.cpp:5492 #4 0x7f5f7deeaaa2 in nsHTMLEditRules::AfterEditInner(EditAction, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:430 #5 0x7f5f7dee8d90 in nsHTMLEditRules::AfterEdit(EditAction, short) src/editor/libeditor/html/nsHTMLEditRules.cpp:376 #6 0x7f5f7de57aac in nsHTMLEditor::EndOperation() src/editor/libeditor/html/nsHTMLEditor.cpp:3513 #7 0x7f5f7d899f1c in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:95 #8 0x7f5f7d877ba2 in ~nsAutoRules src/editor/libeditor/base/nsEditorUtils.h:92 #9 0x7f5f7d87dafc in nsPlaintextEditor::InsertText(nsAString_internal const&) src/editor/libeditor/text/nsPlaintextEditor.cpp:728 #10 0x7f5f7d87df1e in non-virtual thunk to nsPlaintextEditor::InsertText(nsAString_internal const&) src/build/unix/stdc++compat/stdc++compat.cpp:0 #11 0x7f5f7d9995d8 in nsInsertPlaintextCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) src/editor/libeditor/base/nsEditorCommands.cpp:846 #12 0x7f5f80c790d1 in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) src/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:175 #13 0x7f5f80c4c3a2 in nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) src/embedding/components/commandhandler/src/nsBaseCommandController.cpp:153 #14 0x7f5f80c4c676 in non-virtual thunk to nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) src/build/unix/stdc++compat/stdc++compat.cpp:0 #15 0x7f5f80c62c8f in nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) src/embedding/components/commandhandler/src/nsCommandManager.cpp:234 #16 0x7f5f7c1e9498 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) src/content/html/document/src/nsHTMLDocument.cpp:3232 #17 0x7f5f7c1eaded in non-virtual thunk to nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) src/build/unix/stdc++compat/stdc++compat.cpp:0 #18 0x7f5f8438cef7 in NS_InvokeByIndex_P src/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162 #19 0x7f5f7f801e5e in CallMethodHelper::Invoke() src/js/xpconnect/src/XPCWrappedNative.cpp:3106 #20 0x7f5f7f8649f5 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) src/js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1478 #21 0x7f5f8aeeb731 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:372 #22 0x7f5f8ae78821 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2413 #23 0x7f5f8addf815 in js::RunScript(JSContext*, JSScript*, js::StackFrame*) src/js/src/jsinterp.cpp:309 #24 0x7f5f8aef8a46 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:494 #25 0x7f5f8aefa9fe in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:531 #26 0x7f5f8a652034 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5665 #27 0x7f5f8a656fd1 in JS_EvaluateUCScriptForPrincipalsVersionOrigin src/js/src/jsapi.cpp:5746 #28 0x7f5f7c981e3f in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1497 #29 0x7f5f7cb319cf in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9590 #30 0x7f5f7cae9349 in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9851 #31 0x7f5f7cb2fa2a in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10118 #32 0x7f5f842ccbc2 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473 #33 0x7f5f842ce478 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556 #34 0x7f5f84291f99 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:625 #35 0x7f5f83f34017 in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220 #36 0x7f5f82d10bd5 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82 #37 0x7f5f8453ea79 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208 #38 0x7f5f8453e8c2 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201 #39 0x7f5f8453e7a7 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175 #40 0x7f5f821dadee in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163 #41 0x7f5f80e4dc88 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:273 #42 0x7f5f776596f0 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3800 #43 0x7f5f7765f964 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3877 #44 0x7f5f77662a2e in XRE_main src/toolkit/xre/nsAppRunner.cpp:3953 #45 0x40c5bb in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174 #46 0x409e1f in main src/browser/app/nsBrowserApp.cpp:279 #47 0x7f5f9470ac4d in ?? ??:0 0x7f5f56a1dc9d is located 0 bytes to the right of 1053-byte region [0x7f5f56a1d880,0x7f5f56a1dc9d) allocated by thread T0 here: #0 0x4c3ef0 in __interceptor_malloc ??:0 #1 0x7f5f915a86c6 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:57 #2 0x7f5f843106d2 in NS_Alloc_P src/xpcom/base/nsMemoryImpl.cpp:163 #3 0x7f5f777ad7d2 in nsMemory::Alloc(unsigned long) src/../../../dist/include/nsMemory.h:36 #4 0x7f5f7acbea88 in nsTextFragment::SetTo(unsigned short const*, int, bool) src/content/base/src/nsTextFragment.cpp:264 #5 0x7f5f7aa69561 in nsGenericDOMDataNode::SetTextInternal(unsigned int, unsigned int, unsigned short const*, unsigned int, bool, CharacterDataChangeInfo::Details*) src/content/base/src/nsGenericDOMDataNode.cpp:307 #6 0x7f5f7aa776f8 in nsGenericDOMDataNode::SetText(unsigned short const*, unsigned int, bool) src/content/base/src/nsGenericDOMDataNode.cpp:850 #7 0x7f5f78bda486 in nsIContent::SetText(nsAString_internal const&, bool) src/../../../../dist/include/nsIContent.h:513 #8 0x7f5f7a487a84 in CompressWhitespace(nsIContent*) src/layout/mathml/nsMathMLTokenFrame.cpp:98 #9 0x7f5f7a4872dc in nsMathMLTokenFrame::Init(nsIContent*, nsIFrame*, nsIFrame*) src/layout/mathml/nsMathMLTokenFrame.cpp:111 #10 0x7f5f78beb992 in nsCSSFrameConstructor::InitAndRestoreFrame(nsFrameConstructorState const&, nsIContent*, nsIFrame*, nsIFrame*, nsIFrame*, bool) src/layout/base/nsCSSFrameConstructor.cpp:4545 #11 0x7f5f78c1b334 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:3647 #12 0x7f5f78c33520 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:5551 #13 0x7f5f78bec989 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:9817 #14 0x7f5f78beeb58 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsIFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) src/layout/base/nsCSSFrameConstructor.cpp:9961 #15 0x7f5f78c04094 in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsStyleDisplay const*, nsIContent*, nsIFrame*, nsIFrame*, nsStyleContext*, nsIFrame**, nsFrameItems&, bool, PendingBinding*) src/layout/base/nsCSSFrameConstructor.cpp:11008 #16 0x7f5f78c27c40 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsIFrame*, nsStyleDisplay const*, nsFrameItems&, nsIFrame**) src/layout/base/nsCSSFrameConstructor.cpp:4522 #17 0x7f5f78c1a74e in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:3611 #18 0x7f5f78c33520 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:5551 #19 0x7f5f78bec989 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*, nsFrameItems&) src/layout/base/nsCSSFrameConstructor.cpp:9817 #20 0x7f5f78c50f02 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) src/layout/base/nsCSSFrameConstructor.cpp:7218 #21 0x7f5f78c4a8fa in nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) src/layout/base/nsCSSFrameConstructor.cpp:6837 #22 0x7f5f78c3af4d in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool) src/layout/base/nsCSSFrameConstructor.cpp:9341 #23 0x7f5f78c6a537 in nsCSSFrameConstructor::ProcessRestyledFrames(nsStyleChangeList&) src/layout/base/nsCSSFrameConstructor.cpp:8084 #24 0x7f5f78bbb933 in mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) src/layout/base/RestyleTracker.cpp:132 Shadow byte and word: 0x1febead43b93: 5 0x1febead43b90: 00 00 00 05 fb fb fb fb More shadow bytes: 0x1febead43b70: 00 00 00 00 00 00 00 00 0x1febead43b78: 00 00 00 00 00 00 00 00 0x1febead43b80: 00 00 00 00 00 00 00 00 0x1febead43b88: 00 00 00 00 00 00 00 00 =>0x1febead43b90: 00 00 00 05 fb fb fb fb 0x1febead43b98: fb fb fb fb fb fb fb fb 0x1febead43ba0: fa fa fa fa fa fa fa fa 0x1febead43ba8: fa fa fa fa fa fa fa fa 0x1febead43bb0: fa fa fa fa fa fa fa fa Stats: 238M malloced (274M for red zones) by 500040 calls Stats: 42M realloced by 21762 calls Stats: 203M freed by 265667 calls Stats: 71M really freed by 156628 calls Stats: 448M (114766 full pages) mmaped in 112 calls mmaps by size class: 8:294894; 9:40955; 10:16380; 11:14329; 12:3072; 13:1536; 14:1280; 15:256; 16:448; 17:1248; 18:160; 19:40; 20:16; mallocs by size class: 8:406418; 9:50541; 10:17092; 11:17141; 12:2998; 13:1978; 14:1488; 15:324; 16:525; 17:1297; 18:179; 19:42; 20:17; frees by size class: 8:189390; 9:41298; 10:13781; 11:13967; 12:2077; 13:1668; 14:1286; 15:282; 16:466; 17:1284; 18:115; 19:39; 20:14; rfrees by size class: 8:117290; 9:19910; 10:7974; 11:9114; 12:582; 13:521; 14:398; 15:147; 16:339; 17:319; 18:28; 19:5; 20:1; Stats: malloc large: 1535 small slow: 2173 ==30534== ABORTING

Aryeh, do you have time to work on this?

I can't guarantee it, so better someone else do it.

This is very very bad. Here's what happens. In nsHTMLEditRules::WillInsertText, we read the selection's start node and offset. The start node is a text node and the size and offset are both 1054. Then we call nsEditor::IsPreformatted which flushes. That flush causes nsMathMLTokenFrame::Init, which calls CompressWhitespace using the stack below, which changes the size of the textnode to 1053. Later on when we return to WillInsertText, the size 1054 is past the end of the buffer, but we don't know, since we naively assume that the size is still valid. #0 nsTextFragment::ReleaseText (this=0x11afdb5e8) at /Users/ehsanakhgari/moz/tmp/content/base/src/nsTextFragment.cpp:92 #1 0x0000000101af813d in nsTextFragment::SetTo (this=0x11afdb5e8, aBuffer=0x1003e0008, aLength=1053, aUpdateBidi=true) at /Users/ehsanakhgari/moz/tmp/content/base/src/nsTextFragment.cpp:190 #2 0x0000000101a902d6 in nsGenericDOMDataNode::SetTextInternal (this=0x11afdb580, aOffset=0, aCount=1054, aBuffer=0x1003e0008, aLength=1053, aNotify=false, aDetails=0x0) at /Users/ehsanakhgari/moz/tmp/content/base/src/nsGenericDOMDataNode.cpp:307 #3 0x0000000101a923ee in nsGenericDOMDataNode::SetText (this=0x11afdb580, aBuffer=0x1003e0008, aLength=1053, aNotify=false) at /Users/ehsanakhgari/moz/tmp/content/base/src/nsGenericDOMDataNode.cpp:850 #4 0x0000000101584ee3 in nsIContent::SetText (this=0x11afdb580, aStr=@0x7fff5fbf6348, aNotify=false) at nsIContent.h:513 #5 0x0000000101980964 in CompressWhitespace (aContent=0x1180afb60) at /Users/ehsanakhgari/moz/tmp/layout/mathml/nsMathMLTokenFrame.cpp:98 #6 0x0000000101980839 in nsMathMLTokenFrame::Init (this=0x11ad6e040, aContent=0x1180afb60, aParent=0x11b0b63d0, aPrevInFlow=0x0) at /Users/ehsanakhgari/moz/tmp/layout/mathml/nsMathMLTokenFrame.cpp:111 #7 0x0000000101568ee0 in nsCSSFrameConstructor::InitAndRestoreFrame (this=0x1180ac800, aState=@0x7fff5fbf7558, aContent=0x1180afb60, aParentFrame=0x11b0b63d0, aPrevInFlow=0x0, aNewFrame=0x11ad6e040, aAllowCounters=true) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:4545 #8 0x000000010157023f in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x1180ac800, aItem=@0x11b384f20, aState=@0x7fff5fbf7558, aParentFrame=0x11b0b63d0, aFrameItems=@0x7fff5fbf6ce8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:3647 #9 0x000000010157378c in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x1180ac800, aState=@0x7fff5fbf7558, aIter=@0x7fff5fbf6860, aParentFrame=0x11b0b63d0, aFrameItems=@0x7fff5fbf6ce8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:5551 #10 0x00000001015856a3 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x1180ac800, aState=@0x7fff5fbf7558, aItems=@0x7fff5fbf6b38, aParentFrame=0x11b0b63d0, aFrameItems=@0x7fff5fbf6ce8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:9817 #11 0x0000000101569805 in nsCSSFrameConstructor::ProcessChildren (this=0x1180ac800, aState=@0x7fff5fbf7558, aContent=0x116f6d2c0, aStyleContext=0x11aed9e30, aFrame=0x11b0b63d0, aCanHaveGeneratedContent=true, aFrameItems=@0x7fff5fbf6ce8, aAllowBlockStyles=true, aPendingBinding=0x0, aPossiblyLeafFrame=0x11b0b63d0) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:9961 #12 0x000000010156ce8c in nsCSSFrameConstructor::ConstructBlock (this=0x1180ac800, aState=@0x7fff5fbf7558, aDisplay=0x11b39c4b8, aContent=0x116f6d2c0, aParentFrame=0x11aed9c80, aContentParentFrame=0x11aed9c80, aStyleContext=0x11aed9e30, aNewFrame=0x7fff5fbf70f8, aFrameItems=@0x7fff5fbf74d8, aAbsPosContainer=false, aPendingBinding=0x0) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:11008 #13 0x0000000101570e3e in nsCSSFrameConstructor::ConstructNonScrollableBlock (this=0x1180ac800, aState=@0x7fff5fbf7558, aItem=@0x11b384d70, aParentFrame=0x11aed9c80, aDisplay=0x11b39c4b8, aFrameItems=@0x7fff5fbf74d8, aNewFrame=0x7fff5fbf70f8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:4522 #14 0x000000010156ffe4 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x1180ac800, aItem=@0x11b384d70, aState=@0x7fff5fbf7558, aParentFrame=0x11aed9c80, aFrameItems=@0x7fff5fbf74d8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:3611 #15 0x000000010157378c in nsCSSFrameConstructor::ConstructFramesFromItem (this=0x1180ac800, aState=@0x7fff5fbf7558, aIter=@0x7fff5fbf7250, aParentFrame=0x11aed9c80, aFrameItems=@0x7fff5fbf74d8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:5551 #16 0x00000001015856a3 in nsCSSFrameConstructor::ConstructFramesFromItemList (this=0x1180ac800, aState=@0x7fff5fbf7558, aItems=@0x7fff5fbf74f8, aParentFrame=0x11aed9c80, aFrameItems=@0x7fff5fbf74d8) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:9817 #17 0x0000000101577efd in nsCSSFrameConstructor::ContentRangeInserted (this=0x1180ac800, aContainer=0x1180af8a0, aStartChild=0x116f6d2c0, aEndChild=0x0, aFrameState=0x1180ae760, aAllowLazyConstruction=false) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:7218 #18 0x00000001015766e8 in nsCSSFrameConstructor::ContentInserted (this=0x1180ac800, aContainer=0x1180af8a0, aChild=0x116f6d2c0, aFrameState=0x1180ae760, aAllowLazyConstruction=false) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:6837 #19 0x0000000101574575 in nsCSSFrameConstructor::RecreateFramesForContent (this=0x1180ac800, aContent=0x116f6d2c0, aAsyncInsert=false) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:9341 ---Type <return> to continue, or q <return> to quit--- #20 0x000000010157c63c in nsCSSFrameConstructor::ProcessRestyledFrames (this=0x1180ac800, aChangeList=@0x7fff5fbf7af0) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:8084 #21 0x00000001015628c2 in mozilla::css::RestyleTracker::ProcessOneRestyle (this=0x1180ac8e8, aElement=0x116f6d2c0, aRestyleHint=0, aChangeHint=nsChangeHint_ReconstructFrame) at /Users/ehsanakhgari/moz/tmp/layout/base/RestyleTracker.cpp:131 #22 0x00000001015618ab in mozilla::css::RestyleTracker::DoProcessRestyles (this=0x1180ac8e8) at /Users/ehsanakhgari/moz/tmp/layout/base/RestyleTracker.cpp:209 #23 0x00000001015893a9 in mozilla::css::RestyleTracker::ProcessRestyles (this=0x1180ac8e8) at RestyleTracker.h:68 #24 0x000000010158333b in nsCSSFrameConstructor::ProcessPendingRestyles (this=0x1180ac800) at /Users/ehsanakhgari/moz/tmp/layout/base/nsCSSFrameConstructor.cpp:12061 #25 0x000000010162f423 in PresShell::FlushPendingNotifications (this=0x11ad29510, aType=Flush_Style) at /Users/ehsanakhgari/moz/tmp/layout/base/nsPresShell.cpp:3853 #26 0x0000000101825935 in nsComputedDOMStyle::GetStyleContextForElement (aElement=0x1180afb60, aPseudo=0x0, aPresShell=0x11ad29510) at /Users/ehsanakhgari/moz/tmp/layout/style/nsComputedDOMStyle.cpp:284 #27 0x00000001022f4746 in nsEditor::IsPreformatted (this=0x11afcfc00, aNode=0x11afdb5f8, aResult=0x7fff5fbf8ed3) at /Users/ehsanakhgari/moz/tmp/editor/libeditor/base/nsEditor.cpp:4047 #28 0x000000010240e065 in nsHTMLEditRules::WillInsertText (this=0x11b0dc000, aAction=insertText, aSelection=0x11aed1390, aCancel=0x7fff5fbf9177, aHandled=0x7fff5fbf9176, inString=0x7fff5fbf9320, outString=0x7fff5fbf91d0, aMaxLength=-1) at /Users/ehsanakhgari/moz/tmp/editor/libeditor/html/nsHTMLEditRules.cpp:1327 #29 0x000000010240d5a4 in nsHTMLEditRules::WillDoAction (this=0x11b0dc000, aSelection=0x11aed1390, aInfo=0x7fff5fbf9178, aCancel=0x7fff5fbf9177, aHandled=0x7fff5fbf9176) at /Users/ehsanakhgari/moz/tmp/editor/libeditor/html/nsHTMLEditRules.cpp:588 #30 0x00000001022d21a7 in nsPlaintextEditor::InsertText (this=0x11afcfc00, aStringToInsert=@0x7fff5fbf9320) at /Users/ehsanakhgari/moz/tmp/editor/libeditor/text/nsPlaintextEditor.cpp:716 #31 0x00000001022d235f in non-virtual thunk to nsPlaintextEditor::InsertText(nsAString_internal const&) () at /Users/ehsanakhgari/moz/tmp/editor/libeditor/text/nsPlaintextEditor.cpp:728 #32 0x0000000102303f38 in nsInsertPlaintextCommand::DoCommandParams (this=0x117dec3a0, aCommandName=0x7fff5fbf9760 "cmd_insertText", aParams=0x11b3ea150, refCon=0x11afcfc00) at /Users/ehsanakhgari/moz/tmp/editor/libeditor/base/nsEditorCommands.cpp:846 #33 0x0000000102aab5ca in nsControllerCommandTable::DoCommandParams (this=0x117cd2580, aCommandName=0x7fff5fbf9760 "cmd_insertText", aParams=0x11b3ea150, aCommandRefCon=0x11afcfc00) at /Users/ehsanakhgari/moz/tmp/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:175 #34 0x0000000102aa3d76 in nsBaseCommandController::DoCommandWithParams (this=0x1180fd920, aCommand=0x7fff5fbf9760 "cmd_insertText", aParams=0x11b3ea150) at /Users/ehsanakhgari/moz/tmp/embedding/components/commandhandler/src/nsBaseCommandController.cpp:153 #35 0x0000000102aa3dd7 in non-virtual thunk to nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) () at /Users/ehsanakhgari/moz/tmp/embedding/components/commandhandler/src/nsBaseCommandController.cpp:154 #36 0x0000000102aa7d1d in nsCommandManager::DoCommand (this=0x11aed2580, aCommandName=0x7fff5fbf9760 "cmd_insertText", aCommandParams=0x11b3ea150, aTargetWindow=0x117878800) at /Users/ehsanakhgari/moz/tmp/embedding/components/commandhandler/src/nsCommandManager.cpp:234 #37 0x0000000101eb90f1 in nsHTMLDocument::ExecCommand (this=0x11ad36800, commandID=@0x7fff5fbf9cb0, doShowUI=false, value=@0x7fff5fbf9cc8, _retval=0x7fff5fbf9ad8) at /Users/ehsanakhgari/moz/tmp/content/html/document/src/nsHTMLDocument.cpp:3232 #38 0x0000000101eb933e in non-virtual thunk to nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) () at /Users/ehsanakhgari/moz/tmp/content/html/document/src/nsHTMLDocument.cpp:3479 #39 0x000000010346fad9 in NS_InvokeByIndex_P (that=0x11ad36d88, methodIndex=125, paramCount=4, params=0x7fff5fbf9a90) at /Users/ehsanakhgari/moz/tmp/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162 (More stack frames follow...) I can wallpaper over this bug in various ways, but the real question is, how can we solve this in the general case? This really means that we cannot trust information that we keep around about text nodes anywhere in Gecko after we flush. :( The idea of modifying the content tree as part of the layout doesn't seem very appealing. Can't we do something so that this CompressWhitespace call is not needed? Like for example, storing the offsets of the "compressed" whitespaces and handle them in the mathml code whenever this compression is needed?

Note that as far as the rating on this bug goes, this particular instance is a buffer overflow which would not be exploitable, but modifying the content tree during layout like this is so scary that I'm willing to call this sec-critical.

> I can wallpaper over this bug in various ways, but the real question is, how > can we solve this in the general case? This really means that we cannot > trust information that we keep around about text nodes anywhere in Gecko > after we flush. :( Yeah, it is pretty evil. A quick solution would be to move the CompressWhitespace(aContent) call to a scriptrunner so it only runs when it's safe to run script. Still grotesque, but safe. A better solution would be to add a flag on the text frame children of nsMathMLTokenFrame that forces the text frame to trim off leading and trailing whitespace. Leading can be trimmed where we test "if (atStartOfLine && !textStyle->WhiteSpaceIsSignificant())". Trailing can be trimmed where we test canTrimTrailingWhitespace.

A flush can typically trigger XBL constructors/destructors (arguably a bug, but.....). So yeah, normally when you flush you can't trust anything after that point. :(

Yes, the best solution is to trim off leading/trailing spaces when the text frames are displayed/measured. I think the fact that whitespaces are ignored is only important for visual rendering. However, we may also need it in nsMathMLmoFrame, where we try to find the character in the operator dictionary. For other related or similar issues with the MathML token elements, see also bug 785956.

This implements the better solution that roc suggested in comment 5. The MathML reftests seem to pass with it. Frédéric, it would really help if you could please test this patch, as I know practically nothing about MathML. :-) Thanks!

Thanks, I'll give it a try asap. I'll also read the MathML code again to see if there are places where the CompressWhitespace are necessary. In the meantime, can you check that <math><mo minsize="10em"> ( </mo></math> (with leading/trailing spaces) renders the same as <math><mo minsize="10em">(</mo></math>. (you may need https://developer.mozilla.org/en-US/docs/Mozilla_MathML_Project/Fonts)

Fixed comment 9 and a few other similar cases, and added reftests.

Yet more tests!

Fixed a small mistake in the reftest.

Comment on attachment 656525 [details] [diff] [review] Patch (v3) Review of attachment 656525 [details] [diff] [review]: ----------------------------------------------------------------- OK, the reftests and changes in the MathML code look good to me. I don't know what's the problem with <ms>, but I think we can live with it for now. We should probably rewrite the code for <ms> in a cleaner way anyway (bug 560100).

Comment on attachment 656525 [details] [diff] [review] Patch (v3) Review of attachment 656525 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsTextFrame.h @@ +22,5 @@ > #define TEXT_HAS_NONCOLLAPSED_CHARACTERS NS_FRAME_STATE_BIT(31) > > +// This state bit is set on frames which are forced to trim their leading and > +// trailing whitespaces > +#define TEXT_FORCE_TRIM_WHITESPACE NS_FRAME_STATE_BIT(32) From nsIFrame.h: // Bits 20-31 and 60-63 of the frame state are reserved for implementations. #define NS_FRAME_IMPL_RESERVED nsFrameState(0xF0000000FFF00000) So I think you need bit 62. ::: layout/generic/nsTextFrameThebes.cpp @@ +7774,5 @@ > bool usedHyphenation; > gfxFloat trimmedWidth = 0; > gfxFloat availWidth = aAvailableWidth; > + bool canTrimTrailingWhitespace = !textStyle->WhiteSpaceIsSignificant() || > + (GetStateBits() & TEXT_FORCE_TRIM_WHITESPACE); There's a block later in Reflow where we have bool brokeText = forceBreak >= 0 || transformedCharsFit < transformedLength; if (canTrimTrailingWhitespace) { if (brokeText) { AddStateBits(TEXT_TRIMMED_TRAILING_WHITESPACE); } else { textMetrics.mAdvanceWidth += trimmedWidth; I think you need to do something there to ensure we don't take the path that adds trimmedWidth to the advance width. Not sure why this didn't show up in tests. ::: layout/mathml/nsMathMLTokenFrame.cpp @@ +100,5 @@ > + // trailing whitespaces. > + for (nsIFrame* childFrame = GetFirstPrincipalChild(); childFrame; > + childFrame = childFrame->GetNextSibling()) { > + if (childFrame->GetType() == nsGkAtoms::textFrame) { > + childFrame->AddStateBits(TEXT_FORCE_TRIM_WHITESPACE); Need to do something similar in AppendFrames/InsertFrames.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > > I think you need to do something there to ensure we don't take the path that > adds trimmedWidth to the advance width. Not sure why this didn't show up in > tests. > Maybe because the MathML frames use their own metrics measurement and positioning. How about <table> <tr><td style="border: 1px solid black; padding: 0;"><math><mrow><mtext> X</mtext></mrow></math></td></tr> </table> (based on attachment 306114 [details])

cc'ing Karl, as the work on this bug may actually give an idea about how to fix bug 415413.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14) > Comment on attachment 656525 [details] [diff] [review] > Patch (v3) > > Review of attachment 656525 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/nsTextFrame.h > @@ +22,5 @@ > > #define TEXT_HAS_NONCOLLAPSED_CHARACTERS NS_FRAME_STATE_BIT(31) > > > > +// This state bit is set on frames which are forced to trim their leading and > > +// trailing whitespaces > > +#define TEXT_FORCE_TRIM_WHITESPACE NS_FRAME_STATE_BIT(32) > > From nsIFrame.h: > > // Bits 20-31 and 60-63 of the frame state are reserved for implementations. > #define NS_FRAME_IMPL_RESERVED > nsFrameState(0xF0000000FFF00000) > > So I think you need bit 62. Hmm, all of those bits are used in nsTextFrameThebes.cpp. :( Does that mean that I have no free bits to use? > ::: layout/generic/nsTextFrameThebes.cpp > @@ +7774,5 @@ > > bool usedHyphenation; > > gfxFloat trimmedWidth = 0; > > gfxFloat availWidth = aAvailableWidth; > > + bool canTrimTrailingWhitespace = !textStyle->WhiteSpaceIsSignificant() || > > + (GetStateBits() & TEXT_FORCE_TRIM_WHITESPACE); > > There's a block later in Reflow where we have > > bool brokeText = forceBreak >= 0 || transformedCharsFit < > transformedLength; > if (canTrimTrailingWhitespace) { > if (brokeText) { > AddStateBits(TEXT_TRIMMED_TRAILING_WHITESPACE); > } else { > textMetrics.mAdvanceWidth += trimmedWidth; > > I think you need to do something there to ensure we don't take the path that > adds trimmedWidth to the advance width. Not sure why this didn't show up in > tests. OK. > ::: layout/mathml/nsMathMLTokenFrame.cpp > @@ +100,5 @@ > > + // trailing whitespaces. > > + for (nsIFrame* childFrame = GetFirstPrincipalChild(); childFrame; > > + childFrame = childFrame->GetNextSibling()) { > > + if (childFrame->GetType() == nsGkAtoms::textFrame) { > > + childFrame->AddStateBits(TEXT_FORCE_TRIM_WHITESPACE); > > Need to do something similar in AppendFrames/InsertFrames. Done.

It looks possible to overload bit 32: nsIFrame.h:#define NS_FRAME_IS_PUSHED_FLOAT NS_FRAME_STATE_BIT(32) because we currently only check for NS_FRAME_IS_PUSHED_FLOAT on frames that we already know are out-of-flow (we even know it's a float in most cases). We could add TEXT_FORCE_TRIM_WHITESPACE next to PUSHED_FLOAT and explain that it must only be used on text frames. (and vice versa for PUSHED_FLOAT) Otherwise, just reserve four more bits at the top? bits 56-59 are not in use, afaict.

(In reply to Mats Palmgren [:mats] from comment #18) > It looks possible to overload bit 32: > nsIFrame.h:#define NS_FRAME_IS_PUSHED_FLOAT NS_FRAME_STATE_BIT(32) > > because we currently only check for NS_FRAME_IS_PUSHED_FLOAT > on frames that we already know are out-of-flow (we even know > it's a float in most cases). Oh, well, this explains why my patch did not explode on try. :-) > We could add TEXT_FORCE_TRIM_WHITESPACE next to PUSHED_FLOAT and > explain that it must only be used on text frames. (and vice versa > for PUSHED_FLOAT) OK, great. So, roc, can you please review based on using bit 32, and if that's not OK, r-? If it's OK I can make this adjustment when landing. > Otherwise, just reserve four more bits at the top? > bits 56-59 are not in use, afaict. There seems to be no central place to query the used bits... I'll leave this decision to roc.

Comment on attachment 656968 [details] [diff] [review] Patch (v4) Review of attachment 656968 [details] [diff] [review]: ----------------------------------------------------------------- Also add a comment to nsIFrame.h indicating that bit 32 is overloaded by nsTextFrameThebes. r+ with those. ::: layout/generic/nsTextFrameThebes.cpp @@ +7846,5 @@ > // having to re-do it.) > if (brokeText) { > // We're definitely going to break so our trailing whitespace should > // definitely be timmed. Record that we've already done it. > AddStateBits(TEXT_TRIMMED_TRAILING_WHITESPACE); I think we should add this state bit when TEXT_FORCE_TRIM_WHITESPACE is present. Probably doesn't matter in practice but it seems like the right thing to do.

> // definitely be timmed. Record that we've already done it. s/timmed/trimmed/

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20) > Comment on attachment 656968 [details] [diff] [review] > Patch (v4) > > Review of attachment 656968 [details] [diff] [review]: > ----------------------------------------------------------------- > > Also add a comment to nsIFrame.h indicating that bit 32 is overloaded by > nsTextFrameThebes. > > r+ with those. > > ::: layout/generic/nsTextFrameThebes.cpp > @@ +7846,5 @@ > > // having to re-do it.) > > if (brokeText) { > > // We're definitely going to break so our trailing whitespace should > > // definitely be timmed. Record that we've already done it. > > AddStateBits(TEXT_TRIMMED_TRAILING_WHITESPACE); > > I think we should add this state bit when TEXT_FORCE_TRIM_WHITESPACE is > present. Probably doesn't matter in practice but it seems like the right > thing to do. You mean setting both TEXT_FORCE_TRIM_WHITESPACE and TEXT_TRIMMED_TRAILING_WHITESPACE in the MathML code?

No, add TEXT_TRIMMED_TRAILING_WHITESPACE in Reflow.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d53e36893452 So, now the question is, how risky is this patch for branches? It's really hard for me to come up with a risk assessment here...

I'd say it's not very risky since the main risk is to MathML which is a seldom-used feature.

We're going to beta 3 on Tuesday 9/11 and that would be early enough to take this sort of risk, so if this can land on branches before then we're in good shape.

Comment on attachment 657099 [details] [diff] [review] Patch (v5) Review of attachment 657099 [details] [diff] [review]: ----------------------------------------------------------------- Risk discussion in bug.

This broke the ESR build <https://tbpl.mozilla.org/php/getParsedLog.php?id=15062661&tree=Mozilla-Esr10>, so I backed it out: https://hg.mozilla.org/releases/mozilla-esr10/rev/80b1eb27ce42

Seems like I had converted two lines in the patch to all lower case by accident when fixing the merge conflicts. Fixed that and relanded: https://hg.mozilla.org/releases/mozilla-esr10/rev/52e77bb4e86f

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e519ec1916f Sorry about not getting this landed on beta before 9/11 :-(. I'll follow up in email about that.

Bounty awarded $3000

This issue appears to be an issue Qanalyst is unable to verify. Marking QAExclude in QA Whiteboard.