stylo: AddressSanitizer: heap-use-after-free in [@ GetExistingSlots] with READ of size 8

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: truber, Assigned: bholley)

Tracking

(Blocks 2 bugs, 5 keywords)

Trunk
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56+ fixed, firefox57+ fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

Posted file testcase.html
The attached testcase causes a use-after-free crash in m-c rev 44121dbcac6a with stylo enabled by pref.

Either a fuzzing build [1] with fuzzing.enabled=true, or the domFuzzLite extension [2] are required to trigger cycle-collection.

1. https://tools.taskcluster.net/index/artifacts/gecko.v2.mozilla-central.latest.firefox/linux64-fuzzing-asan-opt
2. https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension

==32685==ERROR: AddressSanitizer: heap-use-after-free on address 0x610000053a90 at pc 0x7f52927c2bd2 bp 0x7f523c4931d0 sp 0x7f523c4931c8
READ of size 8 at 0x610000053a90 thread T29 (StyleThread#1)
    #0 0x7f52927c2bd1 in GetExistingSlots /home/worker/workspace/build/src/dom/base/nsINode.h:1945:12
    #1 0x7f52927c2bd1 in GetExistingDOMSlots /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FragmentOrElement.h:390
    #2 0x7f52927c2bd1 in GetExistingExtendedDOMSlots /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/FragmentOrElement.h:405
    #3 0x7f52927c2bd1 in mozilla::dom::Element::GetSMILOverrideStyleDeclaration() /home/worker/workspace/build/src/dom/base/Element.cpp:2027
    #4 0x7f529779fc48 in Gecko_GetSMILOverrideDeclarationBlock /home/worker/workspace/build/src/layout/style/ServoBindings.cpp:496:42
    #5 0x7f529d9b31c1 in style::gecko::wrapper::{{impl}}::get_smil_override /home/worker/workspace/build/src/servo/components/style/gecko/wrapper.rs:883
    #6 0x7f529d9b31c1 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::match_primary::hf9c108ef69a03273 /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:340
    #7 0x7f529d9aadf7 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_primary_style::h865e57febbb64917 /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:96
    #8 0x7f529d9a7617 in _$LT$style..style_resolver..StyleResolverForElement$LT$$u27$a$C$$u20$$u27$ctx$C$$u20$$u27$le$C$$u20$E$GT$$GT$::resolve_style::hf238718fcb092dbe /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:144
    #9 0x7f529d9a1262 in style::style_resolver::{{impl}}::resolve_style_with_default_parents::{{closure}}<style::gecko::wrapper::GeckoElement> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:178
    #10 0x7f529d9a1262 in style::style_resolver::with_default_parent_styles<style::gecko::wrapper::GeckoElement,closure,style::data::ElementStyles> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:66
    #11 0x7f529d9a1262 in style::style_resolver::{{impl}}::resolve_style_with_default_parents<style::gecko::wrapper::GeckoElement> /home/worker/workspace/build/src/servo/components/style/style_resolver.rs:177
    #12 0x7f529d9a1262 in style::traversal::compute_style::h00a4eb976c467bf8 /home/worker/workspace/build/src/servo/components/style/traversal.rs:684
    #13 0x7f529d9bd040 in style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly,closure> /home/worker/workspace/build/src/servo/components/style/traversal.rs:501
    #14 0x7f529d9bd040 in style::gecko::traversal::{{impl}}::process_preorder<closure> /home/worker/workspace/build/src/servo/components/style/gecko/traversal.rs:39
    #15 0x7f529d9bd040 in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /home/worker/workspace/build/src/servo/components/style/parallel.rs:205
    #16 0x7f529d9bd040 in style::parallel::traverse_nodes::h43680bf12ea94af1 /home/worker/workspace/build/src/servo/components/style/parallel.rs:283
    #17 0x7f529d9bd570 in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /home/worker/workspace/build/src/servo/components/style/parallel.rs:221
    #18 0x7f529d9bd570 in style::parallel::traverse_nodes::h43680bf12ea94af1 /home/worker/workspace/build/src/servo/components/style/parallel.rs:283
    #19 0x7f529d9bc713 in style::parallel::traverse_dom::{{closure}}::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> /home/worker/workspace/build/src/servo/components/style/parallel.rs:92
    #20 0x7f529d9bc713 in rayon_core::scope::{{impl}}::execute_job_closure::{{closure}}<closure,()> /home/worker/workspace/build/src/third_party/rust/rayon-core/src/scope/mod.rs:354
    #21 0x7f529d9bc713 in std::panic::{{impl}}::call_once<(),closure> /checkout/src/libstd/panic.rs:296
    #22 0x7f529d9bc713 in std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> /checkout/src/libstd/panicking.rs:454
    #23 0x7f529d9bc713 in panic_abort::__rust_maybe_catch_panic /checkout/src/libpanic_abort/lib.rs:40
Flags: in-testsuite?
Posted file log.txt
It appears that the <a> element (generated by turning on designMode on a table) has been freed.
Assignee

Comment 3

2 years ago
(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #2)
> It appears that the <a> element (generated by turning on designMode on a
> table) has been freed.

Is the <a> element some sort of NAC? If so, then perhaps the issue is that we're finding that element via the frame tree (via the StyleChildrenIterator), but the frame has a stale reference to the dead element?
Assignee

Updated

2 years ago
Priority: -- → P1
Yeah, it seems to be created when HTMLEditor::ShowInlineTableEditingUI calls HTMLEditor::CreateAnonymousElement[1]

[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/editor/libeditor/HTMLInlineTableEditor.cpp#64
Assignee

Comment 5

2 years ago
Hm, so it looks like the StyleChildrenIterator finds this NAC via the manualNACProperty machinery. So this is the machinery that got added in bug 1374999. Assuming that's correct, then this is orthogonal to the frame tree, and just a question of how we manage to drop the ref to the NAC without also de-registering it in DeleteRefToAnonymousNode. Maybe this is some edge-case where the PresShell is null? DeleteRefToAnonymousNode should spit out a warning in that case.

As belt-and-suspenders, we should probably make ManualNAC hold strong references to the elements.

Brian, are you going to continue investigating this?
Flags: needinfo?(bbirtles)
No, sorry I have been at standards meetings all week and am away from now until mid-next week.
Flags: needinfo?(bbirtles)
Assignee

Comment 7

2 years ago
Ok, I'll take it then.
Assignee: nobody → bobbyholley
Assignee

Comment 8

2 years ago
OK, so the existing code is only correct if HTMLEditor::HideInlineTableEditingUI is called before the destructor runs, but it isn't. This is a regression from bug 1374999.
Blocks: 1374999
Assignee

Comment 9

2 years ago
(This probably affects non-stylo as well)
Assignee

Comment 11

2 years ago
This is belt-and-suspenders. It transforms the current UAF into a leak, which
is strictly preferable.

MozReview-Commit-ID: H1W7RZeGQYy
Attachment #8893112 - Flags: review?(masayuki)
Assignee

Comment 12

2 years ago
MozReview-Commit-ID: BxbmWjovaHm
Attachment #8893114 - Flags: review?(masayuki)
Assignee

Comment 13

2 years ago
Posted patch Crashtest.Splinter Review
MozReview-Commit-ID: DGFMbL3Djrq
Attachment #8893115 - Flags: review+
Comment on attachment 8893112 [details] [diff] [review]
Part 1 - Use a strong reference for ManualNAC. v1

I'm not a good person to review nsContentUtils. Please look for better reviewer.
Attachment #8893112 - Flags: review?(masayuki)
Comment on attachment 8893112 [details] [diff] [review]
Part 1 - Use a strong reference for ManualNAC. v1

Ah, but it'll be moved to editor. Okay, I'll review this.
Attachment #8893112 - Flags: review?(masayuki)
Assignee

Comment 16

2 years ago
(You can also ask heycam, who added the original ManualNAC machinery - I just figured that you might want to review the new setup for editor).
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d6f9510a73a14aa0905e7dece30657318d6b13cc

Isn't it a bad practice to push patch of sec-critical bug to try server before getting sec-approval?

https://wiki.mozilla.org/Security/Bug_Approval_Process#Pushing_to_Try
Assignee

Comment 18

2 years ago
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #17)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #10)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=d6f9510a73a14aa0905e7dece30657318d6b13cc
> 
> Isn't it a bad practice to push patch of sec-critical bug to try server
> before getting sec-approval?
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process#Pushing_to_Try

I don't think it's realistic to develop complex patches against mozilla-central without pushing to try.

I certainly could have obfuscated it more, and not pushed the crashtest. But since it doesn't affect release and I expect to have the patch landed within a day or two, I'm not particularly worried about it.
Comment on attachment 8893114 [details] [diff] [review]
Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v1

># HG changeset patch
># User Bobby Holley <bobbyholley@gmail.com>
>
>Bug 1386110 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v1
>
>MozReview-Commit-ID: BxbmWjovaHm
>
>diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp
>index 8173c1d6..f8dfabf 100644
>--- a/dom/base/nsContentUtils.cpp
>+++ b/dom/base/nsContentUtils.cpp
>@@ -33,16 +33,17 @@
> #include "mozilla/ArrayUtils.h"
> #include "mozilla/Attributes.h"
> #include "mozilla/AutoRestore.h"
> #include "mozilla/AutoTimelineMarker.h"
> #include "mozilla/Base64.h"
> #include "mozilla/CheckedInt.h"
> #include "mozilla/DebugOnly.h"
> #include "mozilla/LoadInfo.h"
>+#include "mozilla/ManualNAC.h"
> #include "mozilla/dom/ContentChild.h"

nit: This file includes header files a-z order of the *path*. So, "mozilla/Ma" should be inserted to below |#include "mozilla/Likely.h"|.

>diff --git a/editor/libeditor/ManualNAC.h b/editor/libeditor/ManualNAC.h
>+#ifndef mozilla_ManualNAC_h
>+#define mozilla_ManualNAC_h
>+
>+#include "mozilla/dom/Element.h"

I think that you need to include RefPtr.h too for avoiding include hell.

>+
>+namespace mozilla {
>+
>+// 16 seems to be the maximum number of manual NAC nodes that editor
>+// creates for a given element.

I don't know what "NAC" means. If you know, could you add the original words with parentheses?

(I wonder, "NAC" is "Native Anonymous Content"?)

>+//
>+// These need to be manually removed by the machinery that sets the NAC,
>+// otherwise we'll leak.
>+typedef AutoTArray<RefPtr<mozilla::dom::Element>,16> ManualNACArray;

nit: please insert a whitespace before "16". And perhaps, "mozilla::" isn't necessary here.

>+
>+class ManualNACPtr

final?

And please add explanation of the purpose of this class before this line.

>+{
>+public:
>+  ManualNACPtr() {}
>+  ManualNACPtr(decltype(nullptr)) {}
>+  ManualNACPtr(already_AddRefed<Element> aNewNAC)

Shouldn't be a reference?

>+  // We use move semantics, and delete the copy-constructor and operator=.
>+  ManualNACPtr(ManualNACPtr&& aOther) : mPtr(aOther.mPtr.forget()) {}
>+  ManualNACPtr(ManualNACPtr& aOther) = delete;
>+  ManualNACPtr& operator=(ManualNACPtr&& aOther) { mPtr = aOther.mPtr.forget(); return *this; }

too long line. Plase wrap after |aOther)|, |{|, |forget();|, |*this;| and |}|.

>+  Element* Get() const { return mPtr.get(); }

nit: |get()| may be better than |Get()| for consistency with RefPtr and nsCOMPtr.

>+  Element* operator->() const { return Get(); }
>+  operator Element*() const &
>+  {
>+    return Get();
>+  }
>+
>+private:
>+  RefPtr<Element> mPtr;
>+};
>+
>+} // namespace mozilla
>+
>+inline void
>+ImplCycleCollectionUnlink(mozilla::ManualNACPtr& field)
>+{
>+    field.Reset();

Use 2 spaces for indent.

>+}
>+
>+inline void
>+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& callback,
>+                            const mozilla::ManualNACPtr& field,
>+                            const char* name,
>+                            uint32_t flags = 0)
>+{
>+    CycleCollectionNoteChild(callback, field.Get(), name, flags);

Use 2 spaces for indent.

>diff --git a/editor/libeditor/HTMLAbsPositionEditor.cpp b/editor/libeditor/HTMLAbsPositionEditor.cpp
>@@ -284,20 +284,18 @@ HTMLEditor::HideGrabber()
>   NS_ENSURE_TRUE(mGrabber, NS_ERROR_NULL_POINTER);
> 
>   // get the presshell's document observer interface.
>   nsCOMPtr<nsIPresShell> ps = GetPresShell();
>   // We allow the pres shell to be null; when it is, we presume there
>   // are no document observers to notify, but we still want to
>   // UnbindFromTree.
> 
>-  DeleteRefToAnonymousNode(mGrabber, ps);
>-  mGrabber = nullptr;
>-  DeleteRefToAnonymousNode(mPositioningShadow, ps);
>-  mPositioningShadow = nullptr;
>+  DeleteRefToAnonymousNode(Move(mGrabber), ps);
>+  DeleteRefToAnonymousNode(Move(mPositioningShadow), ps);

If the caller forgets to use Move(), will it cause compile error due to no copy constructor? I worry about easy mistake of other developers including me.


I'd like to check new patch, temporarily r-.
Attachment #8893114 - Flags: review?(masayuki) → review-
Assignee

Comment 20

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #19)

> nit: This file includes header files a-z order of the *path*. So,
> "mozilla/Ma" should be inserted to below |#include "mozilla/Likely.h"|.

Fixed.

> 
> >diff --git a/editor/libeditor/ManualNAC.h b/editor/libeditor/ManualNAC.h
> >+#ifndef mozilla_ManualNAC_h
> >+#define mozilla_ManualNAC_h
> >+
> >+#include "mozilla/dom/Element.h"
> 
> I think that you need to include RefPtr.h too for avoiding include hell.

Fixed.

> 
> >+
> >+namespace mozilla {
> >+
> >+// 16 seems to be the maximum number of manual NAC nodes that editor
> >+// creates for a given element.
> 
> I don't know what "NAC" means. If you know, could you add the original words
> with parentheses?

Fixed.

> 
> (I wonder, "NAC" is "Native Anonymous Content"?)

Yes.

> 
> >+//
> >+// These need to be manually removed by the machinery that sets the NAC,
> >+// otherwise we'll leak.
> >+typedef AutoTArray<RefPtr<mozilla::dom::Element>,16> ManualNACArray;
> 
> nit: please insert a whitespace before "16". And perhaps, "mozilla::" isn't
> necessary here.

Fixed.

> 
> >+
> >+class ManualNACPtr
> 
> final?

Sure.

> 
> And please add explanation of the purpose of this class before this line.

Done.

> 
> >+{
> >+public:
> >+  ManualNACPtr() {}
> >+  ManualNACPtr(decltype(nullptr)) {}
> >+  ManualNACPtr(already_AddRefed<Element> aNewNAC)
> 
> Shouldn't be a reference?

Hm, is that preferable? Seems like that just allows aliasing the already_AddRefed, whereas passing by value requires the caller to use already_AddRefed's move-constructor, either by passing an anonymous result or by explicitly Move()-ing a value.

> 
> >+  // We use move semantics, and delete the copy-constructor and operator=.
> >+  ManualNACPtr(ManualNACPtr&& aOther) : mPtr(aOther.mPtr.forget()) {}
> >+  ManualNACPtr(ManualNACPtr& aOther) = delete;
> >+  ManualNACPtr& operator=(ManualNACPtr&& aOther) { mPtr = aOther.mPtr.forget(); return *this; }
> 
> too long line. Plase wrap after |aOther)|, |{|, |forget();|, |*this;| and
> |}|.

Fixed.

> 
> >+  Element* Get() const { return mPtr.get(); }
> 
> nit: |get()| may be better than |Get()| for consistency with RefPtr and
> nsCOMPtr.

Ok.

> 
> >+  Element* operator->() const { return Get(); }
> >+  operator Element*() const &
> >+  {
> >+    return Get();
> >+  }
> >+
> >+private:
> >+  RefPtr<Element> mPtr;
> >+};
> >+
> >+} // namespace mozilla
> >+
> >+inline void
> >+ImplCycleCollectionUnlink(mozilla::ManualNACPtr& field)
> >+{
> >+    field.Reset();
> 
> Use 2 spaces for indent.

Fixed (was cargo-culting from another file).

> 
> >+}
> >+
> >+inline void
> >+ImplCycleCollectionTraverse(nsCycleCollectionTraversalCallback& callback,
> >+                            const mozilla::ManualNACPtr& field,
> >+                            const char* name,
> >+                            uint32_t flags = 0)
> >+{
> >+    CycleCollectionNoteChild(callback, field.Get(), name, flags);
> 
> Use 2 spaces for indent.

Fixed.

> 
> >diff --git a/editor/libeditor/HTMLAbsPositionEditor.cpp b/editor/libeditor/HTMLAbsPositionEditor.cpp
> >@@ -284,20 +284,18 @@ HTMLEditor::HideGrabber()
> >   NS_ENSURE_TRUE(mGrabber, NS_ERROR_NULL_POINTER);
> > 
> >   // get the presshell's document observer interface.
> >   nsCOMPtr<nsIPresShell> ps = GetPresShell();
> >   // We allow the pres shell to be null; when it is, we presume there
> >   // are no document observers to notify, but we still want to
> >   // UnbindFromTree.
> > 
> >-  DeleteRefToAnonymousNode(mGrabber, ps);
> >-  mGrabber = nullptr;
> >-  DeleteRefToAnonymousNode(mPositioningShadow, ps);
> >-  mPositioningShadow = nullptr;
> >+  DeleteRefToAnonymousNode(Move(mGrabber), ps);
> >+  DeleteRefToAnonymousNode(Move(mPositioningShadow), ps);
> 
> If the caller forgets to use Move(), will it cause compile error due to no
> copy constructor?

That's right, since the copy-constructor is explicitly |delete|.
Assignee

Comment 21

2 years ago
MozReview-Commit-ID: HTSu5BjxD8I
Attachment #8893501 - Flags: review?(masayuki)
Assignee

Updated

2 years ago
Attachment #8893114 - Attachment is obsolete: true
Assignee

Comment 22

2 years ago
Comment on attachment 8893501 [details] [diff] [review]
Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v2

Flagging sec approval for both patches in the bug, since I expect v2 to be r+ed.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not super-easily, would need to figure out how to trigger the crash, and then exploit it. Crashtest does the first, but we can wait to push that (accidentally pushed it to try though).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It's probably clear that we're dealing with UAF.

Which older supported branches are affected by this flaw?

56 and 57. Not 55.

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

bug 1386110.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Low risk, same patch should apply.

How likely is this patch to cause regressions; how much testing does it need?

We should land it on Nightly, wait a day or two to make sure there's no major fallout, then land to beta.
Attachment #8893501 - Flags: sec-approval?
Comment on attachment 8893501 [details] [diff] [review]
Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v2

>> >+{
>> >+public:
>> >+  ManualNACPtr() {}
>> >+  ManualNACPtr(decltype(nullptr)) {}
>> >+  ManualNACPtr(already_AddRefed<Element> aNewNAC)
>> 
>> Shouldn't be a reference?
> 
> Hm, is that preferable? Seems like that just allows aliasing the
> already_AddRefed, whereas passing by value requires the caller to use
> already_AddRefed's move-constructor, either by passing an anonymous result or
> by explicitly Move()-ing a value.

I thought that not using reference causes unnecessary |.take()| is called. If it's not matter or it's my misunderstanding, keep using this style. (Anyway, really a small issue.)
Attachment #8893501 - Flags: review?(masayuki) → review+
Assignee

Comment 24

2 years ago
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #23)
> I thought that not using reference causes unnecessary |.take()| is called.

In the sense that the Move constructor is invoked where it otherwise wouldn't be? That is true, but I'm pretty sure that modern compilers can optimize out the actual invocation of the Move constructor in cases like this (since it's just shuffling the pointer between temporaries, with no refcounting). Let me know if we have any evidence that it does add overhead.

Thanks for the reviews!
Thank you for fixing this Bobby.  BTW you probably need some explicit / MOZ_IMPLICT annotations on the ManualNACPtr constructors.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #22)
> If not all supported branches, which bug introduced the flaw?
> 
> bug 1386110.

Probably you mean bug 1374999.
Comment on attachment 8893501 [details] [diff] [review]
Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. v2

sec-approval= dveditz
Attachment #8893501 - Flags: sec-approval? → sec-approval+
Attachment #8893112 - Flags: sec-approval+
Assignee

Updated

2 years ago
Attachment #8893501 - Attachment is obsolete: true
Assignee

Comment 28

2 years ago
Comment on attachment 8893651 [details] [diff] [review]
Part 2 - Use a smart pointer to reliably de-register NAC regardless of how it goes away. r=masayuki

Applies to Part 1 + Part 2.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1374999
[User impact if declined]: UAF
[Is this code covered by automated tests?]: Yes (crashtest in the bug)
[Has the fix been verified in Nightly?]: Not yet (just pushed)
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: Part 1 + Part 2 together
[Is the change risky?]: medium-low.
[Why is the change risky/not risky?]: Reshuffles some editor internals, but the behavior should be the same.
[String changes made/needed]: None
Attachment #8893651 - Flags: approval-mozilla-beta?
Assignee

Updated

2 years ago
Attachment #8893112 - Flags: approval-mozilla-beta?
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #24)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #23)
> > I thought that not using reference causes unnecessary |.take()| is called.
> 
> In the sense that the Move constructor is invoked where it otherwise
> wouldn't be? That is true, but I'm pretty sure that modern compilers can
> optimize out the actual invocation of the Move constructor in cases like
> this (since it's just shuffling the pointer between temporaries, with no
> refcounting). Let me know if we have any evidence that it does add overhead.
> 
> Thanks for the reviews!

No problem. Thank you for your explanation. I still don't understand around Move of C++11 deeply.
https://hg.mozilla.org/mozilla-central/rev/72756e10985d
https://hg.mozilla.org/mozilla-central/rev/e79cd7f08b4f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: dom-core-security → core-security-release
Comment on attachment 8893112 [details] [diff] [review]
Part 1 - Use a strong reference for ManualNAC. v1

Crash fix, sec issue, let's uplift this for 56 beta 2.
Attachment #8893112 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8893651 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #28)
> [Is this code covered by automated tests?]: Yes (crashtest in the bug)
> [Has the fix been verified in Nightly?]: Not yet (just pushed)
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Bobby's assessment on manual testing needs.
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.