Closed
Bug 842166
Opened 11 years ago
Closed 11 years ago
crash [@ mozilla::ScrollbarActivity::CancelActivityFinishedTimer() ] in destroyed frame
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: w3bd3vil, Assigned: MatsPalmgren_bugz)
References
Details
(5 keywords, Whiteboard: [adv-main22-])
Crash Data
Attachments
(4 files, 5 obsolete files)
438 bytes,
text/html
|
Details | |
2.39 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1019 bytes,
patch
|
ehsan.akhgari
:
review+
akeybl
:
approval-mozilla-aurora-
|
Details | Diff | Splinter Review |
21.65 KB,
text/html
|
Details |
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 Build ID: 20130201065344 Steps to reproduce: <html> <head> <style> li{ display: table-footer-group; } </style> <meta HTTP-EQUIV="Cache-Control" content="no-cache" /> </head> <body> <script> <ins> </ins> </script> <li contenteditable="true"> </li> <object type="checkbox"> </object> <select> </select> </body> </html> Actual results: 0:000> r eax=00000000 ebx=f0de7fff ecx=7e26b03a edx=00000000 esi=f0de7fff edi=03120280 eip=6786c8d7 esp=0053a698 ebp=0053a774 iopl=0 nv up ei ng nz na pe nc cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00210286 xul!nsListScrollSmoother::Stop+0x5: 6786c8d7 8b4608 mov eax,dword ptr [esi+8] ds:0023:f0de8007=???????? 0:000> kb ChildEBP RetAddr Args to Child 0053a698 67a1b6af f0de7fff 05004e70 67629daa xul!nsListScrollSmoother::Stop+0x5 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\xul\base\src\nslistboxbodyframe.cpp @ 126] 0053a6a4 67629daa f0de7fff 04d605fc 05007fb0 xul!mozilla::ScrollbarActivity::ActivityOccurred+0xb [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\generic\scrollbaractivity.cpp @ 22] 0053a774 6745992a 04d605fc 00000000 00000000 xul!nsGfxScrollFrameInner::SetCoordAttribute+0x1a345a [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\generic\nsgfxscrollframe.cpp @ 3634] 0053a7e0 67440f67 0641a800 076a4480 0641a800 xul!nsGfxScrollFrameInner::ReflowFinished+0x1ea [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\generic\nsgfxscrollframe.cpp @ 3400] 0053a7f8 6741eb04 076a4400 00000000 0641a800 xul!PresShell::HandlePostedReflowCallbacks+0xe7 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\base\nspresshell.cpp @ 3708] 0053a850 6741e777 00000000 076a4480 06398c00 xul!PresShell::ProcessReflowCommands+0x254 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\base\nspresshell.cpp @ 7686] 0053a8fc 675947ae 00000005 05007da0 076a57a0 xul!PresShell::FlushPendingNotifications+0x1d7 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\layout\base\nspresshell.cpp @ 3878] 0053a924 6734329f 00000005 0053a9b0 00000000 xul!nsDocument::FlushPendingNotifications+0xce [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\base\src\nsdocument.cpp @ 6527] 0053a94c 6759e1d2 076a57a0 0053a9b0 0053a988 xul!nsGenericElement::GetBoundingClientRect+0x5e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\base\src\nsgenericelement.cpp @ 797] 0053a960 673cebba 076a5888 00000044 00000001 xul!NS_InvokeByIndex_P+0x27 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp @ 71] 0053ab44 673cc6ba 00000000 00000000 0053ac04 xul!XPCWrappedNative::CallMethod+0x39a [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\xpconnect\src\xpcwrappednative.cpp @ 2383] 0053abcc 68a435b9 05026000 00000000 03b10188 xul!XPC_WN_CallMethod+0xaa [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\xpconnect\src\xpcwrappednativejsops.cpp @ 1469] 0053ac28 68a4786f 05026000 00000000 03b10198 mozjs!js::InvokeKernel+0x59 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 365] 0053b778 68a42f7e 03b100b0 00000000 00000000 mozjs!js::Interpret+0x9ef [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 2473] 0053b7ec 68a43791 05026000 053a8634 03b100b0 mozjs!js::RunScript+0x8e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 322] 0053b848 68a43488 05026000 00000000 03b100a8 mozjs!js::InvokeKernel+0x231 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 376] 0053b888 68a3a751 05026000 0053b8b0 0053b8cc mozjs!js::Invoke+0x128 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 409] 0053b8bc 673cfdd4 05026000 0539db20 053a8620 mozjs!JS_CallFunctionValue+0x41 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsapi.cpp @ 5889] 0053bb1c 675949a7 05b80190 0599c200 00000003 xul!nsXPCWrappedJSClass::CallMethod+0x494 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\xpconnect\src\xpcwrappedjsclass.cpp @ 1434] 0053bb44 6759e2d0 0599c200 00000003 031d9418 xul!nsXPCWrappedJS::CallMethod+0x47 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\xpconnect\src\xpcwrappedjs.cpp @ 580] 0053bc04 6759e344 059bae10 00000003 0053bc2c xul!PrepareAndDispatch+0xfa [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 85] 0053bc20 673f0fd6 059bae10 07696370 0623fc40 xul!SharedStub+0x16 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\reflect\xptcall\src\md\win32\xptcstubs.cpp @ 113] 0053bc78 673f1507 05b76ec0 05961400 0623fc40 xul!nsEventListenerManager::HandleEventInternal+0x2e6 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\events\src\nseventlistenermanager.cpp @ 964] 0053bcc4 67405a40 0053bd00 00000006 00000000 xul!nsEventTargetChainItem::HandleEventTargetChain+0x2a7 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\events\src\nseventdispatcher.cpp @ 286] 0053bd84 67437017 076a57a0 05961400 0623fc40 xul!nsEventDispatcher::Dispatch+0x500 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\events\src\nseventdispatcher.cpp @ 636] 0053bdb8 67452989 076a57a0 00000000 05961400 xul!nsEventDispatcher::DispatchDOMEvent+0x67 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\events\src\nseventdispatcher.cpp @ 691] 0053bdd8 6737bfed 00000000 07696370 0053be0b xul!nsINode::DispatchEvent+0x59 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\base\src\nsinode.cpp @ 1078] 0053be0c 68a435b9 05919c00 00000001 03b10060 xul!nsIDOMEventTarget_DispatchEvent+0x83 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\obj-firefox\js\xpconnect\src\dom_quickstubs.cpp @ 10400] 0053be68 68a4786f 05919c00 00000000 03b10070 mozjs!js::InvokeKernel+0x59 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 365] 0053c9a8 68a42f7e 03b10020 00000000 00000000 mozjs!js::Interpret+0x9ef [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 2473] 0053ca1c 68a43791 05919c00 0459ca94 03b10020 mozjs!js::RunScript+0x8e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 322] 0053ca78 68a43488 05919c00 00000000 03b10020 mozjs!js::InvokeKernel+0x231 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 376] 0053cab8 68a3a751 05919c00 0053cae0 0053cafc mozjs!js::Invoke+0x128 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsinterp.cpp @ 409] 0053caec 6744ce71 05919c00 0458e4c0 0459ca80 mozjs!JS_CallFunctionValue+0x41 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\js\src\jsapi.cpp @ 5889] 0053cb50 6744c059 05ba69b0 076a57a0 062c1f00 xul!nsXBLProtoImplAnonymousMethod::Execute+0x131 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\xbl\src\nsxblprotoimplmethod.cpp @ 331] 0053cb68 674046e8 00000000 06398c00 0053cb8c xul!nsXBLBinding::ExecuteAttachedHandler+0x49 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\xbl\src\nsxblbinding.cpp @ 1103] 0053cb8c 673feb2b 050072f8 05b0b3f0 00000008 xul!nsDocument::MaybeEndOutermostXBLUpdate+0x178 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\base\src\nsdocument.cpp @ 4152] 0053cbac 6744075a 00b031f0 05b0b3f0 063d2bfc xul!nsHTMLDocument::EndUpdate+0x7b [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\content\html\document\src\nshtmldocument.cpp @ 2353] 0053cbc0 6740f7b5 00b86ec4 04c55320 00b20100 xul!nsHtml5TreeOpExecutor::EndDocUpdate+0x9a [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\parser\html\nshtml5treeopexecutor.h @ 248] 0053cbf0 67329109 6742e625 04c55320 00b410e0 xul!nsHtml5TreeOpExecutor::RunFlushLoop+0x1b5 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\parser\html\nshtml5treeopexecutor.cpp @ 593] 0053cbf4 6742e625 04c55320 00b410e0 00b0e3d0 xul!nsHtml5ExecutorReflusher::Run+0xc [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\parser\html\nshtml5treeopexecutor.cpp @ 67] 0053cc60 6754879f 00b20101 00000000 0053cc9c xul!nsThread::ProcessNextEvent+0x1b5 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\xpcom\threads\nsthread.cpp @ 626] 0053cc94 6753152c 00b41001 7e757cf6 00b20100 xul!mozilla::ipc::MessagePump::Run+0x5f [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\glue\messagepump.cpp @ 82] 0053cccc 675314d4 00000001 67496600 00000000 xul!MessageLoop::RunHandler+0x21 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\chromium\src\base\message_loop.cc @ 209] 0053cce8 67529c1a 80000000 00b86ec0 67531580 xul!MessageLoop::Run+0x15 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\ipc\chromium\src\base\message_loop.cc @ 183] 0053ccf4 67531580 00b86ec0 00b172c0 00db0000 xul!nsBaseAppShell::Run+0x34 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\widget\xpwidgets\nsbaseappshell.cpp @ 165] 0053ec48 67576e5b 00b86ec0 00b172c0 672fe4e4 xul!nsAppShell::Run+0x4e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\widget\windows\nsappshell.cpp @ 232] 0053ec54 672fe4e4 00b172c0 00000000 6ccf10a0 xul!nsAppStartup::Run+0x1e [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\components\startup\nsappstartup.cpp @ 291] 0053ed24 675155c3 0053ed60 00db32e8 0053ed60 xul!XREMain::XRE_mainRun+0x405 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nsapprunner.cpp @ 3794] 0053ed40 6753d505 0053ed60 00000001 006d3db0 xul!XREMain::XRE_main+0xde [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nsapprunner.cpp @ 3860] 0053ee58 00db1742 00000001 006d3db0 00db32e8 xul!XRE_main+0x30 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nsapprunner.cpp @ 3935] 0053f930 00db1a64 00000001 006d1160 006d11e0 firefox!wmain+0x742 [e:\builds\moz2_slave\rel-m-rel-w32-bld\build\toolkit\xre\nswindowswmain.cpp @ 105] 0053f974 77351866 7fa77000 0053f9c4 77d468f1 firefox!__tmainCRTStartup+0x122 [f:\dd\vctools\crt_bld\self_x86\crt\src\crtexe.c @ 552] 0053f980 77d468f1 7fa77000 61ae5788 00000000 KERNEL32!BaseThreadInitThunk+0xe 0053f9c4 77d4689d ffffffff 77dd5aea 00000000 ntdll!__RtlUserThreadStart+0x4a
Comment 1•11 years ago
|
||
Firefox 21.0a1 Crash Report [@ mozilla::ScrollbarActivity::CancelActivityFinishedTimer() ] win 7 bp-9d080b36-8a8d-40d0-bbb5-c30232130218 exploitability rating - low linux bp-73ceebf8-3b2f-4100-80eb-9d8652130218
Crash Signature: [@ mozilla::ScrollbarActivity::CancelActivityFinishedTimer() ]
Component: Untriaged → Layout
OS: Windows 8 → All
Product: Firefox → Core
Hardware: x86_64 → All
Comment 2•11 years ago
|
||
Regression range from Linux x86-64 nightlies: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4fddb9923ef0&tochange=bc69705c162d
My first guess was https://hg.mozilla.org/mozilla-central/rev/3a057b8667fb , but that's not it.
Comment 5•11 years ago
|
||
fwiw, bisecting downloaded opt builds on linux x86_64 gave me: Found regression between 20121118030713-20121119030725 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b959971b8219&tochange=4fddb9923ef0 http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/11/2012-11-18-03-07-13-mozilla-central/firefox-19.0a1.en-US.linux-x86_64.tar.bz2 http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/11/2012-11-19-03-07-25-mozilla-central/firefox-19.0a1.en-US.linux-x86_64.tar.bz2 Trying to narrow it with custom builds failed due to xul errors.
Assignee | ||
Comment 6•11 years ago
|
||
Backing out bug 800018 makes it not crash.
Assignee | ||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
So I guess the real culprit is bug 778810 that added the use of mScrollbarActivity in a place where 'this' isn't safe to use. http://hg.mozilla.org/mozilla-central/rev/c449b548784e#l5.92
Blocks: 778810
Assignee | ||
Comment 9•11 years ago
|
||
BTW, does nsGfxScrollFrameInner::UpdateScrollbarPosition() have the same problem? Which affects ScrollToImpl, ScrollToWithOrigin, ScrollBy ...
Comment 10•11 years ago
|
||
Bug 778810 landed in 17, so setting flags appropriately.
Status: UNCONFIRMED → NEW
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → affected
status-b2g18-v1.0.1:
--- → affected
status-firefox18:
--- → wontfix
status-firefox19:
--- → wontfix
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox-esr17:
--- → affected
Ever confirmed: true
Assignee | ||
Comment 11•11 years ago
|
||
I think frame poisoning mitigates this from being exploitable.
Keywords: csec-framepoisoning,
sec-other
Assignee | ||
Comment 12•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=634c1ca11579
Assignee: nobody → matspal
Updated•11 years ago
|
Flags: sec-bounty?
Assignee | ||
Comment 13•11 years ago
|
||
Make SetCoordAttribute static, and return true if the attribute value changed. Move the "mScrollbarActivity->ActivityOccurred()" to the (two) consumers. Add WeakFrame.IsAlive() checks to everything that reach SetCoordAttribute or SetScrollbarEnabled. (I considered using nsAutoScriptBlocker instead but I suspect it would break stuff if I add it on a high enough level that it would make a difference) https://tbpl.mozilla.org/?tree=Try&rev=9f8991c9e8f2 Hopefully this will also fix our nsGfxScrollFrameInner::ScrollTo* crashes.
Attachment #715280 -
Attachment is obsolete: true
Attachment #715755 -
Flags: review?(roc)
I don't like this approach, it seems too fragile to have to deal with frame destruction in all these places. Can we make ActivityOccurred run off AddScriptRunner?
Assignee | ||
Comment 15•11 years ago
|
||
Me neither ;-) but there are numerous other potential crash bugs here, not just the reference to mScrollbarActivity. Even with ActivityOccurred running off an AddScriptRunner the amount of added WeakFrame checks would be roughly the same. We can only avoid all that by calling SetAttr with aNotify=false, but that's not possible AIUI. Or is it?
What exactly can those SetAttrs trigger that can reconstruct frames?
Reporter | ||
Comment 17•11 years ago
|
||
Just a bump, been a while since there has been any update on this.
Comment 18•11 years ago
|
||
Our current understanding is that frame-poisoning is an effective mitigation so not awarding a bug bounty
Flags: sec-bounty? → sec-bounty-
Assignee | ||
Comment 19•11 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16) > What exactly can those SetAttrs trigger that can reconstruct frames? I think the attached stack already answers that to some degree. In the absence of a ScriptBlocker, which I assume we can't use here, it executes script runners, which may cause document mutations causing mutation observer methods to run. My experience is that any call that has aNotify=true generally means it can destroy any (non-ref-counted) layout data and ref-counted data too unless you have an explicit kungFuDeathGrip on it. I would love to be corrected on this if it's not true. Maybe Boris can give us a more precise answer though?
Flags: needinfo?(bzbarsky)
Comment 20•11 years ago
|
||
The basic problem I know of is in fact that stack: where a totally unrelated EndUpdate suddenly triggers editor instantiation....
Flags: needinfo?(bzbarsky)
Comment 21•11 years ago
|
||
Yes, basically anywhere we notify something, there is a chance of the editor being instantiated and that can trigger all sorts of DOM mutations and frame constructions. Mats, why can't we just avoid notifying here?
Assignee | ||
Comment 22•11 years ago
|
||
IIRC, we already tried not notifying in some older crash bug and it broke scrolling somehow. I have a vague memory of CurPosAttributeChanged not being called when we wanted... I can try again and see what breaks I suppose, problem is our test coverage here is not great.
Assignee | ||
Comment 23•11 years ago
|
||
Boris, what stuff do we have that runs off an nsAsyncDOMEvent? It seems it can create all sorts of havoc... note the SetAttr in stack frame #72.
Comment 24•11 years ago
|
||
Um... we run all sorts of stuff from nsAsyncDOMEvent, either via runnables or scriptrunners. The stack there around #72 is just showing that SetAttr can cause an nsAsyncDOMEvent to be posted as a scriptrunner, right?
Assignee | ||
Comment 25•11 years ago
|
||
Yeah, I guess it's not a problem unless we run into something that creates them.
Assignee | ||
Comment 26•11 years ago
|
||
Ehsan, is this what you suggested? It makes the viewport scrollbar unusable.
Comment 27•11 years ago
|
||
Comment on attachment 727498 [details] [diff] [review] SetAttr with aNotify=false Yes, this is what I was suggesting... Sigh :(
Comment 28•11 years ago
|
||
How about making sure that nsTextEditRules::CreateMozBR does not notify? You would probably need to propagate down a boolean to the CreateElementTxn to ask it to not notify if it's set, but I don't know what that could break either... :/ However, if that passes all of our tests, then I don't feel extremely bad about taking that patch.
Comment 29•11 years ago
|
||
As a side note, can you guys who understand the issue fix up the bug summary to be more specific what this one is about?
Assignee | ||
Comment 30•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28) > How about making sure that nsTextEditRules::CreateMozBR does not notify? > You would probably need to propagate down a boolean to the CreateElementTxn > to ask it to not notify if it's set There's no way to prevent nsINode::InsertBefore from notifying mutation observers, afaict. And even if there is, I suspect it would break stuff. Not creating the frame for the inserted fake <br> seems like it would cause blocks with a caret to have zero height?
Summary: Firefox 18.0.2 - Memory Corruption → crash [@ mozilla::ScrollbarActivity::CancelActivityFinishedTimer() ] in destroyed frame
Assignee | ||
Comment 31•11 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #20) > The basic problem I know of is in fact that stack: where a totally unrelated > EndUpdate suddenly triggers editor instantiation.... If editor instantiation is the *only* way SetAttr can cause unrelated frames to be destroyed then I guess we could get away with addressing just that problem. I'm investigating DeferredContentEditableCountChange, MaybeEditingStateChanged and friends...
Assignee | ||
Updated•11 years ago
|
Attachment #715755 -
Attachment is obsolete: true
Attachment #715755 -
Flags: review?(roc)
Assignee | ||
Comment 33•11 years ago
|
||
A few WIPs, each fixes the crash individually... Make editor instantiation async: https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=f8b7f5773082 Still sync, but suppress MaybeEditingStateChanged script runners from a nested scope: https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=59fa830e0d83 Script blocker in nsGfxScrollFrameInner::ReflowFinished: https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=42cce4513fb7 Ehsan, do you see any reason why the first wouldn't work? I think 1 or 2 is required, 3 is additional for extra safety just in case editor instantiation isn't the only way.
Assignee | ||
Updated•11 years ago
|
Attachment #727489 -
Attachment is obsolete: true
Comment 34•11 years ago
|
||
(In reply to Mats Palmgren [:mats] from comment #33) > Ehsan, do you see any reason why the first wouldn't work? No, but it's a bit hard to reason about all of the things which could go wrong. Let's see what TBPL thinks.
Assignee | ||
Comment 35•11 years ago
|
||
The async approach didn't work at all (I guess script is using editor stuff before the editor was instantiated, execCommand and the like). Patch 2 and 3 each passed tests individually so let's go with 2 + 3.
Assignee | ||
Comment 36•11 years ago
|
||
Attachment #727498 -
Attachment is obsolete: true
Attachment #728371 -
Flags: review?(ehsan)
Assignee | ||
Comment 37•11 years ago
|
||
Try run with both patches: https://tbpl.mozilla.org/?tree=Try&rev=10967243754f
Attachment #728372 -
Flags: review?(ehsan)
Comment 38•11 years ago
|
||
Comment on attachment 728371 [details] [diff] [review] Ignore MaybeEditingStateChanged() during nsDocument::EndUpdate since nsHTMLDocument::EndUpdate will do it Review of attachment 728371 [details] [diff] [review]: ----------------------------------------------------------------- You forgot to initialize this new member in the ctor. r=me with that.
Attachment #728371 -
Flags: review?(ehsan) → review+
Updated•11 years ago
|
Attachment #728372 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 39•11 years ago
|
||
> You forgot to initialize this new member in the ctor. r=me with that. Thanks, but it's not necessary: http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#195
Assignee | ||
Comment 40•11 years ago
|
||
For the record, the GetBoundingClientRect in stack frame #73 came from browser.js according DumpJSStack() in a debugger. I wonder if it could be harmful to performance to flush layout like that.
Attachment #715096 -
Attachment is obsolete: true
Assignee | ||
Comment 41•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b065837d7072 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d5cb79e8f8e
Flags: in-testsuite?
Comment 42•11 years ago
|
||
(In reply to Mats Palmgren [:mats] from comment #39) > > You forgot to initialize this new member in the ctor. r=me with that. > > Thanks, but it's not necessary: > http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/ > nsHTMLDocument.cpp#195 Heh, nasty! (In reply to Mats Palmgren [:mats] from comment #40) > Created attachment 728491 [details] > stack > > For the record, the GetBoundingClientRect in stack frame #73 came from > browser.js according DumpJSStack() in a debugger. > > I wonder if it could be harmful to performance to flush layout like that. Yeah, flushing layout is usually harmful for performance. Please file a bug on that
Assignee | ||
Comment 43•11 years ago
|
||
Filed bug 854054.
Comment 44•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5d5cb79e8f8e https://hg.mozilla.org/mozilla-central/rev/b065837d7072
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(In reply to Mats Palmgren [:mats] from comment #19) > My experience is that any call that has aNotify=true generally means it > can destroy any (non-ref-counted) layout data and ref-counted data too > unless you have an explicit kungFuDeathGrip on it. I would love to be > corrected on this if it's not true. That's probably true in general but here we're setting an attribute on specific anonymous content elements which shouldn't have crazy event listeners or malicious XBL or anything like that attached.
Comment 46•11 years ago
|
||
Please nominate this bug for ESR tracking and change the status to affected if you all disagree with wontfixing. Seems like the worst case scenario here isn't very bad.
Updated•11 years ago
|
tracking-b2g18:
--- → -
tracking-firefox-esr17:
--- → -
Assignee | ||
Comment 47•11 years ago
|
||
I think frame poisoning mitigates this from being exploitable so I agree with wontfix for esr17 and b2g18. The fix is low-risk though so it seems worth taking on Aurora. I just checked - it applies cleanly and fixes the crash. There's 250 crash incidents matching the signature on crash-stats in the past 4 week period.
Assignee | ||
Comment 48•11 years ago
|
||
Comment on attachment 728372 [details] [diff] [review] Block scripts during nsGfxScrollFrameInner::ReflowFinished() [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 778810 User impact if declined: crash (non-exploitable) Testing completed (on m-c, etc.): on m-c since 2013-03-23 Risk to taking this patch (and alternatives if risky): low risk String or UUID changes made by this patch: none Request is for both patches.
Attachment #728372 -
Flags: approval-mozilla-aurora?
Comment 49•11 years ago
|
||
Comment on attachment 728372 [details] [diff] [review] Block scripts during nsGfxScrollFrameInner::ReflowFinished() If we wontfix for ESR, we should just let the fix ride the trains for sake of consistency.
Attachment #728372 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Assignee | ||
Updated•11 years ago
|
status-firefox22:
--- → fixed
Target Milestone: --- → mozilla22
Updated•11 years ago
|
Group: core-security
Updated•11 years ago
|
Whiteboard: [adv-main22-]
Assignee | ||
Comment 51•10 years ago
|
||
Landed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/70c9239286e5
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•