Last Comment Bug 865537 - (CVE-2013-1684) Heap-use-after-free in mozilla::dom::HTMLMediaElement::LookupMediaElementURITable
(CVE-2013-1684)
: Heap-use-after-free in mozilla::dom::HTMLMediaElement::LookupMediaElementURIT...
Status: VERIFIED FIXED
[adv-main22+][adv-esr1707+]
: crash, csectype-uaf, sec-critical, testcase
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 All
: -- critical (vote)
: mozilla24
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
: Matt Wobensmith [:mwobensmith][:matt:]
Mentors:
Depends on:
Blocks: 495546
  Show dependency treegraph
 
Reported: 2013-04-24 20:04 PDT by Abhishek Arya
Modified: 2015-10-24 07:37 PDT (History)
17 users (show)
abillings: sec‑bounty+
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
22+
verified
22+
fixed
wontfix
affected
fixed


Attachments
Testcase (1.38 KB, application/java-archive)
2013-04-24 20:04 PDT, Abhishek Arya
no flags Details
Part 1: Add nsRange::SetEnableGravitationOnElementRemoval to suppress 'gravitation' behavior on node removal (3.19 KB, patch)
2013-05-14 23:24 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bugs: review+
mats: review-
Details | Diff | Review
Part 2: The Range used to track the source element pointer for a media element should not gravitate outside the media element (3.56 KB, patch)
2013-05-14 23:26 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
cpearce: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
abillings: sec‑approval+
Details | Diff | Review
Testcase #2 (WARNING: might hang your browser) (474 bytes, text/html)
2013-06-08 08:33 PDT, Mats Palmgren (:mats)
no flags Details
b2g18 part 1 patch (3.07 KB, patch)
2013-06-16 20:39 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
b2g18 part 2 patch (4.66 KB, patch)
2013-06-16 20:40 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bajaj.bhavana: approval‑mozilla‑b2g18+
Details | Diff | Review
esr17 part 1 patch (3.07 KB, patch)
2013-06-16 20:42 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details | Diff | Review
esr17 part 2 patch (4.78 KB, patch)
2013-06-16 20:42 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
bajaj.bhavana: approval‑mozilla‑esr17+
Details | Diff | Review

Description Abhishek Arya 2013-04-24 20:04:28 PDT
Created attachment 741645 [details]
Testcase

Needs fuzzPriv extension to force gc. Let the testcase reload a couple of times and you should see a crash.

==29697== ERROR: AddressSanitizer: heap-use-after-free on address 0x6048007cc018 at pc 0x7f37966c651e bp 0x7fff45e783b0 sp 0x7fff45e783a8
READ of size 8 at 0x6048007cc018 thread T0
    #0 0x7f37966c651d in nsCOMPtr<nsINodeInfo>::get() const objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:782
    #1 0x7f37966c618e in nsCOMPtr<nsINodeInfo>::operator->() const objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsCOMPtr.h:802
    #2 0x7f37966b2b78 in nsINode::NodePrincipal() const objdir-ff-asan-sym/media/webrtc/signaling/signaling_ecc/../../../../dist/include/nsINode.h:744
    #3 0x7f379a06126c in mozilla::dom::HTMLMediaElement::LookupMediaElementURITable(nsIURI*) content/html/content/src/HTMLMediaElement.cpp:1863
    #4 0x7f379a05a23e in mozilla::dom::HTMLMediaElement::LoadResource() content/html/content/src/HTMLMediaElement.cpp:1073
    #5 0x7f379a054443 in mozilla::dom::HTMLMediaElement::LoadFromSourceChildren() content/html/content/src/HTMLMediaElement.cpp:926
    #6 0x7f379a0add22 in mozilla::dom::nsSyncSection::Run() content/html/content/src/HTMLMediaElement.cpp:672
    #7 0x7f379fd84000 in nsBaseAppShell::RunSyncSectionsInternal(bool, unsigned int) widget/xpwidgets/nsBaseAppShell.cpp:352
    #8 0x7f379fd7feca in nsBaseAppShell::RunSyncSections(bool, unsigned int) widget/xpwidgets/nsBaseAppShell.h:89
    #9 0x7f379fd8576d in nsBaseAppShell::AfterProcessNextEvent(nsIThreadInternal*, unsigned int) widget/xpwidgets/nsBaseAppShell.cpp:408
    #10 0x7f379fd859f1 in non-virtual thunk to nsBaseAppShell::AfterProcessNextEvent(nsIThreadInternal*, unsigned int) widget/xpwidgets/nsBaseAppShell.cpp:410
    #11 0x7f37a4062218 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:640
    #12 0x7f37a3d0c532 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
    #13 0x7f37a096c8a9 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #14 0x7f37a4378d1b in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
    #15 0x7f37a4378b6e in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:212
    #16 0x7f37a4378a59 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
    #17 0x7f379fd801ae in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #18 0x7f379ea6da40 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:289
    #19 0x7f3794aadaeb in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3878
    #20 0x7f3794ab31b0 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3945
    #21 0x7f3794ab5e75 in XRE_main toolkit/xre/nsAppRunner.cpp:4146
    #22 0x427c57 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:271
    #23 0x424e11 in main browser/app/nsBrowserApp.cpp:576
    #24 0x7f37b6ac576c in
    #25 0x424584 in
0x6048007cc018 is located 24 bytes inside of 472-byte region [0x6048007cc000,0x6048007cc1d8)
freed by thread T0 here:
    #0 0x4183f2 in __interceptor_free
    #1 0x7f37b483f6ce in moz_free memory/mozalloc/mozalloc.cpp:48
    #2 0x7f379a6e7cc9 in operator delete(void*) ../../../../dist/include/mozilla/mozalloc.h:225
    #3 0x7f379a6e7cc9 in mozilla::dom::HTMLVideoElement::~HTMLVideoElement() content/html/content/src/HTMLVideoElement.cpp:78
    #4 0x7f3798cef9c5 in nsNodeUtils::LastRelease(nsINode*) content/base/src/nsNodeUtils.cpp:258
    #5 0x7f3798f9a2fa in mozilla::dom::FragmentOrElement::Release() content/base/src/FragmentOrElement.cpp:1716
    #6 0x7f379a044d75 in mozilla::dom::HTMLMediaElement::Release() content/html/content/src/HTMLMediaElement.cpp:402
    #7 0x7f379a6e3c85 in mozilla::dom::HTMLVideoElement::Release() content/html/content/src/HTMLVideoElement.cpp:42
    #8 0x7f379dd4e4a0 in _ZL17DoDeferredReleaseIP11nsISupportsEvR8nsTArrayIT_E js/xpconnect/src/XPCJSRuntime.cpp:614
    #9 0x7f379dd4d971 in XPCJSRuntime::GCCallback(JSRuntime*, JSGCStatus) js/xpconnect/src/XPCJSRuntime.cpp:814
    #10 0x7f37ab9e702c in Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:4611
    #11 0x7f37ab9d7924 in js::GC(JSRuntime*, js::JSGCInvocationKind, JS::gcreason::Reason) js/src/jsgc.cpp:4628
    #12 0x7f37ab94c4b9 in JS::GCForReason(JSRuntime*, JS::gcreason::Reason) js/src/jsfriendapi.cpp:180
    #13 0x7f379dc5df7e in nsXPCComponents_Utils::ForceGC() js/xpconnect/src/XPCComponents.cpp:4106
    #14 0x7f37a4182f7b in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #15 0x7f379dea35af in CallMethodHelper::Invoke() js/xpconnect/src/XPCWrappedNative.cpp:2948
    #16 0x7f379dea35af in CallMethodHelper::Call() js/xpconnect/src/XPCWrappedNative.cpp:2282
    #17 0x7f379dea35af in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2248
    #18 0x7f379defc7e1 in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1486
    #19 0x7f37abc38ccf in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:337
    #20 0x7f37abc38ccf in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:407
    #21 0x7f37abc121a0 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2396
    #22 0x7f37abbbd64d in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:364
    #23 0x7f37abc39378 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:421
    #24 0x7f37ab65a836 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) js/src/jsinterp.h:134
    #25 0x7f37abc3cdd0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) js/src/jsinterp.cpp:454
    #26 0x7f37abf6b52d in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:481
    #27 0x7f37ac5ae772 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jswrapper.cpp:453
    #28 0x7f37abfd499e in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:2613
    #29 0x7f37abff1e9a in proxy_Call(JSContext*, unsigned int, JS::Value*) js/src/jsproxy.cpp:3177
    #30 0x7f37abc385c2 in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:337
    #31 0x7f37abc385c2 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/jsinterp.cpp:400
    #32 0x7f37abc121a0 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode, bool) js/src/jsinterp.cpp:2396
    #33 0x7f37abbbd64d in js::RunScript(JSContext*, js::StackFrame*) js/src/jsinterp.cpp:364
    #34 0x7f37abc4275b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) js/src/jsinterp.cpp:552
previously allocated by thread T0 here:
    #0 0x4184d2 in malloc
    #1 0x7f37b483f815 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
    #2 0x7f379a6e34ce in operator new(unsigned long) ../../../../dist/include/mozilla/mozalloc.h:201
    #3 0x7f379a6e34ce in nsGenericHTMLElement* mozilla::dom::NewHTMLElementHelper::Create<nsHTMLVideoElement, mozilla::dom::HTMLVideoElement>(already_AddRefed<nsINodeInfo>, mozilla::dom::NewHTMLElementHelper::SFINAE<bool (*)(nsIDocument*), mozilla::dom::HTMLVideoElement::InNavQuirksMode>*) content/html/content/src/nsGenericHTMLElement.h:1855
    #4 0x7f379a6e32db in NS_NewHTMLVideoElement(already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/html/content/src/HTMLVideoElement.cpp:36
    #5 0x7f379a7a8bb1 in CreateHTMLElement(unsigned int, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/html/document/src/nsHTMLContentSink.cpp:498
    #6 0x7f379a7a9375 in NS_NewHTMLElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/html/document/src/nsHTMLContentSink.cpp:481
    #7 0x7f3798cbe79a in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) content/base/src/nsNameSpaceManager.cpp:192
    #8 0x7f379c6d9ee0 in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) parser/html/nsHtml5TreeOperation.cpp:349
    #9 0x7f379c6f89a5 in nsHtml5TreeOpExecutor::RunFlushLoop() parser/html/nsHtml5TreeOpExecutor.cpp:557
    #10 0x7f379c732fb2 in nsHtml5ExecutorFlusher::Run() parser/html/nsHtml5StreamParser.cpp:125
    #11 0x7f37a4061da7 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:627
    #12 0x7f37a3d0c532 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
    #13 0x7f37a096c8a9 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:82
    #14 0x7f37a4378d1b in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
    #15 0x7f37a4378b6e in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:212
    #16 0x7f37a4378a59 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
    #17 0x7f379fd801ae in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #18 0x7f379ea6da40 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:289
    #19 0x7f3794aadaeb in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3878
    #20 0x7f3794ab31b0 in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3945
    #21 0x7f3794ab5e75 in XRE_main toolkit/xre/nsAppRunner.cpp:4146
    #22 0x427c57 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:271
    #23 0x424e11 in main browser/app/nsBrowserApp.cpp:576
    #24 0x7f37b6ac576c in
Shadow bytes around the buggy address:
  0x0c09800f17b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c09800f17c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c09800f17d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c09800f17e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c09800f17f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c09800f1800: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c09800f1810: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c09800f1820: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c09800f1830: fd fd fd fd fd fd fd fd fd fd fd fa fa fa fa fa
  0x0c09800f1840: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c09800f1850: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 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 righ 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
  ASan internal:         fe
==29697== ABORTING
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-04-25 00:24:43 PDT
Hmm.  If the nsSyncSection is alive, how did the element die???
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2013-04-25 00:25:10 PDT
Unless someone dropped a ref somewhere where they shouldn't have, of course....
Comment 3 [:jesup] on pto until 2016/7/5 Randell Jesup 2013-05-07 01:55:50 PDT
HTMLMediaElement isn't webrtc; it's either DOM (original component) or Audio/Video.  There are no webrtc constructs in the testcase, only <video>.

bizarre testcase (but of course it's fuzzing).

Loading it (without fuzzPriv) causes a serios of assertions:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 215
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 487
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 215
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file ../../../../netwerk/base/src/nsFileStreams.cpp, line 487
###!!! ASSERTION: Should only iterate over direct children: 'startContainer == thisDomNode', file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 3350
WARNING: Load group requested for media element in inactive document.: file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 3406
###!!! ASSERTION: Should only iterate over direct children: 'startContainer == thisDomNode', file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 3350
###!!! ASSERTION: Shouldn't have a decoder: 'mDecoder == nullptr', file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 2330
WARNING: Load group requested for media element in inactive document.: file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 3406
###!!! ASSERTION: Should not have entry for element in element table before addition: 'MediaElementTableCount(this, mLoadingSrc) == 0', file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 1813
###!!! ASSERTION: Should have a single entry for element in element table after addition: 'MediaElementTableCount(this, mLoadingSrc) == 1', file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 1821
###!!! ASSERTION: Media element should have single table entry if decode initialized: 'NS_SUCCEEDED(rv) == (MediaElementTableCount(this, mLoadingSrc) == 1)', file ../../../../../content/html/content/src/HTMLMediaElement.cpp, line 2456
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-05-14 23:24:21 PDT
Created attachment 749681 [details] [diff] [review]
Part 1: Add nsRange::SetEnableGravitationOnElementRemoval to suppress 'gravitation' behavior on node removal
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-05-14 23:26:49 PDT
Created attachment 749683 [details] [diff] [review]
Part 2: The Range used to track the source element pointer for a media element should not gravitate outside the media element

This patch fixes the included crashtest and seems to fix Abhishek's reported testcase as well.
Comment 6 Alex Keybl [:akeybl] 2013-05-15 12:24:02 PDT
mwobensmith - can you confirm whether FF22 is affected?
Comment 7 Matt Wobensmith [:mwobensmith][:matt:] 2013-05-15 14:12:17 PDT
Crashes FF22 with today's build.
Comment 8 Chris Pearce (:cpearce) 2013-05-15 14:29:14 PDT
Comment on attachment 749683 [details] [diff] [review]
Part 2: The Range used to track the source element pointer for a media element should not gravitate outside the media element

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

Looks good, but is it safe to land this patch with comments + crashtest pinpointing a potential exploit?

::: content/html/content/src/HTMLMediaElement.cpp
@@ +3379,5 @@
>    if (!mSourcePointer) {
>      // First time this has been run, create a selection to cover children.
>      mSourcePointer = new nsRange(this);
> +    // If this media element is removed from the DOM, don't gravitate the
> +    // range up to its ancestor, leave it attached to the media element.

+= "so that the load algorithm's iteration over the <source> children isn't broken..."

::: content/media/test/crashtests/865537-1.html
@@ +3,5 @@
> +<body onload="doTest()">
> +<base href="../unknown">
> +<div id="test3"></div>
> +<video id="test4"><source src="white.webm"></video>
> +<script>

Is it safe to land the crashtest when this bug exists in release builds?
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-05-15 15:14:08 PDT
Comment on attachment 749683 [details] [diff] [review]
Part 2: The Range used to track the source element pointer for a media element should not gravitate outside the media element

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Unclear, but possibly easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
They clearly point to a problem. The problem they point to isn't immediately exploitable, but further work could get to an exploit.

Which older supported branches are affected by this flaw?
All.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I expect backporting to be easy, this code hasn't changed much.

How likely is this patch to cause regressions; how much testing does it need?
Very low regression risk.
Comment 10 [On PTO until 6/29] 2013-05-15 20:02:03 PDT
sec-approval+ for checking on 5/28 or after. Please prepare and nominate branch patches when it is in m-c.
Comment 11 Jason Smith [:jsmith] 2013-05-15 20:42:20 PDT
Looks like the qawanted request here has been met. Clearing the flag, unless I'm missing something here.
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-07 00:54:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d91e2d9da8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0285f4360664
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-07 01:03:05 PDT
Comment on attachment 749683 [details] [diff] [review]
Part 2: The Range used to track the source element pointer for a media element should not gravitate outside the media element

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: security bug
Testing completed (on m-c, etc.): just landed on m-c
Risk to taking this patch (and alternatives if risky): very low risk.
String or IDL/UUID changes made by this patch: none
Comment 15 Mats Palmgren (:mats) 2013-06-07 04:07:07 PDT
Comment on attachment 749681 [details] [diff] [review]
Part 1: Add nsRange::SetEnableGravitationOnElementRemoval to suppress 'gravitation' behavior on node removal

I think we still want to do the last part of ContentRemoved (which is
unrelated to fixing up the Range at hand) even if we suppress the
gravitation part.  IOW, replace the first hunk with:

if ((gravitateStart || gravitateEnd) && mEnableGravitationOnElementRemoval)
Comment 16 Mats Palmgren (:mats) 2013-06-07 04:30:50 PDT
Nit: we also gravitate ranges with a text node boundary point when that text
node is removed so "Element" in the flag name should really be "Node".
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-07 14:43:26 PDT
(In reply to Mats Palmgren [:mats] from comment #15)
> I think we still want to do the last part of ContentRemoved (which is
> unrelated to fixing up the Range at hand) even if we suppress the
> gravitation part.  IOW, replace the first hunk with:
> 
> if ((gravitateStart || gravitateEnd) && mEnableGravitationOnElementRemoval)

I wasn't sure. It's only a performance issue right? Nothing bad is going to happen if those state bits remain set?
Comment 20 Mats Palmgren (:mats) 2013-06-08 08:16:19 PDT
It's probably just a performance issue at the moment.  It might trigger
a non-fatal "orphan selection descendant" assertion in debug builds.
I don't see any reason to leave it broken though.

Now that I look more closely at the code I'm actually more worried
about skipping the gravitation.  I wonder if all nsRange.cpp code
that touches boundary points deals with them being !IsInDoc.
Comment 21 Mats Palmgren (:mats) 2013-06-08 08:33:51 PDT
Created attachment 760120 [details]
Testcase #2 (WARNING: might hang your browser)

Loading this test and waiting 10 seconds makes my browser non-responsive.

When the <p> is removed I get 1000 calls to nsRange::ContentRemoved
so the <media> ranges are still alive at that point even though the
elements were removed from the document.  What's keeping them alive?
Is there a bug in the CYCLE_COLLECTION stuff for this element?

Hovering over the menubar, I get errors like:
Error loading icon: Failed to open file '/usr/share/icons/oxygen/16x16/actions/edit-redo.png': Too many open files

Is this a known issue?
Comment 22 Mats Palmgren (:mats) 2013-06-08 11:44:34 PDT
> What's keeping them alive?

I found fuzzPriv.CC() which does indeed collect and destroy them, so never mind.

> Too many open files ... Is this a known issue?

Couldn't find one so I filed bug 881003.
Comment 23 Mihaela Velimiroviciu (:mihaelav) 2013-06-13 02:54:59 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0

Verified the fix on latest firefox beta with DOM Fuzzer 2012.07.07 add-on installed, using the attached cases: firefox doesn't crash and doesn't hang. Marking verified for Firefox 22.
Comment 24 Mihaela Velimiroviciu (:mihaelav) 2013-06-13 07:36:41 PDT
Removing the verified flag, since I'm thinking that my previous comment was not enough to verify this (I used normal, non-debug builds) and that other builds need to be used.
Can someone confirm whether that was enough? If not, can you please provide guidelines on which build should be used for verifying this bug?
Comment 25 Jesse Ruderman 2013-06-13 14:41:07 PDT
This was reported using an ASan build, I'd suggest verifying with one.  It wouldn't hurt to also test a debug build or debug+ASan build.
Comment 26 bhavana bajaj [:bajaj] 2013-06-14 15:24:46 PDT
needsinfo on :roc to help with esr/b2g18 patches needed here.
Comment 27 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-14 17:20:11 PDT
I'll verify with an ASan build on Monday.
Comment 28 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 20:39:57 PDT
Created attachment 763319 [details] [diff] [review]
b2g18 part 1 patch
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 20:40:29 PDT
Created attachment 763320 [details] [diff] [review]
b2g18 part 2 patch
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 20:42:16 PDT
Created attachment 763321 [details] [diff] [review]
esr17 part 1 patch
Comment 31 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 20:42:40 PDT
Created attachment 763322 [details] [diff] [review]
esr17 part 2 patch
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 20:45:22 PDT
Comment on attachment 763320 [details] [diff] [review]
b2g18 part 2 patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): none
User impact if declined: security bug
Testing completed: landed on central
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-06-16 20:45:41 PDT
Comment on attachment 763322 [details] [diff] [review]
esr17 part 2 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: security bug
Fix Landed on Version: central
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 34 Mihaela Velimiroviciu (:mihaelav) 2013-06-17 06:18:41 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20130617 Firefox/24.0, build ID: 20130617031112

Verified using the latest night ASAN build and the steps from bug description: no error messages appear
Comment 35 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-17 11:38:41 PDT
Verified ASan builds Aurora/Beta, 2013-06-14.
Comment 37 Matt Wobensmith [:mwobensmith][:matt:] 2013-06-18 10:42:42 PDT
Verified ASan ESR 2013-06-18.

Note You need to log in before you can comment on or make changes to this bug.