1018486 - Audit the rest of Gecko for problems similar to bug 1005584 where we hold a kungfudeathgrip to an object and call multiple methods on it

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

I discussed this with jst a few weeks ago when I was in MTV in person.  Please see bug 1005584 comment 8 for an explanation of the issue.  We need to audit the rest of the places in Gecko where we hold kungfudeathgrips to make sure that they are not affected by this.

Assigning to Johnny to find an owner.

Comment 1

[Benjamin Smedberg]

The basic pattern in question seems to be:

nsCOMPtr<nsIFoo> kungFuDeathGrip(mSomething);

mSomething->Complicated();
...
mSomething->Complicated();

If mSomething can change, we either need to:

A. call the method on the deathgrip instead of mSomething
B. reset the deathgrip the second time around.

And the correct solution depends on the context of the method. Ehsan is that a reasonable summary of the problem?

Comment 2

[(no longer active)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> The basic pattern in question seems to be:
> 
> nsCOMPtr<nsIFoo> kungFuDeathGrip(mSomething);
> 
> mSomething->Complicated();
> ...
> mSomething->Complicated();
> 
> If mSomething can change, we either need to:
> 
> A. call the method on the deathgrip instead of mSomething
> B. reset the deathgrip the second time around.
> 
> And the correct solution depends on the context of the method. Ehsan is that
> a reasonable summary of the problem?

Yes, precisely.

Comment 3

[Daniel Veditz [:dveditz]]

It seems unlikely we don't have any more of these. sec-audit isn't getting attention so I'm raising the severity to that of bug 1005584 which would be the likely severity of any other such problems.

Comment 4

[Andrew McCreight [:mccr8]]

Johnny, is there somebody who can work on this?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Can we statically analyze our way to victory here? Identifying the places where this happens seems tractable ...

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #5)
> Can we statically analyze our way to victory here? Identifying the places
> where this happens seems tractable ...

I don't see why we couldn't do a static analysis where we detect a kungFuDeathGrip being taken of a member, and then multiple methods being called on the member. Determining exactly what we want to catch probably isn't trivial, but it's definitely doable if we think it's a priority.

Comment 7

[Andrew McCreight [:mccr8]]

Michael could you work on this maybe? Thanks.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

I did a quick grep through the tree to see if I could get an idea of how many cases would need to be checked if we decided to look manually rather than with an automated tool.

This was done by looking for the pattern of a kungFuDeathGrip being taken of a value who's name starts with m[A-Z]:

$ git grep 'kungFuDeathGrip.*\<m[A-Z]'

There are only 105 matches to that pattern in the codebase. I'm not sure that a static analysis, especially one which has poorly defined bounds, will be useful when we only need to analyze 105 kungFuDeathGrips.

Comment 9

[Andrew McCreight [:mccr8]]

In addition to things that look like comment 1, there's a bunch of places that seem to just grab a kungfudeathgrip at the top of a method, then do random stuff, then use the field that was gripped, like this: http://dev.searchfox.org/mozilla-central/source/dom/base/TextInputProcessor.cpp#377

It seems like one has to either (a) audit all of the intermediate methods to see if they change the mFoo method, or (b) move the grip down to where the method is actually called on the field. (I could even imagine some helper class that does the grip, the method call, then immediately clears the pointer to avoid abuse.)  (b) sounds better to me, but of course other code might implicitly depend on the earlier grip.

Ehsan, do you have any thoughts on this?

Comment 10

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #8)
> I did a quick grep through the tree to see if I could get an idea of how
> many cases would need to be checked if we decided to look manually rather
> than with an automated tool.
> 
> This was done by looking for the pattern of a kungFuDeathGrip being taken of
> a value who's name starts with m[A-Z]:
> 
> $ git grep 'kungFuDeathGrip.*\<m[A-Z]'
> 
> There are only 105 matches to that pattern in the codebase. I'm not sure
> that a static analysis, especially one which has poorly defined bounds, will
> be useful when we only need to analyze 105 kungFuDeathGrips.

I think the value from the static analysis would be to ensure that the second called to Complicated() in comment 1 doesn't get introduced without people realizing that this problem could now be a sec-critical in the code after such a change.

(In reply to Andrew McCreight [:mccr8] from comment #9)
> In addition to things that look like comment 1, there's a bunch of places
> that seem to just grab a kungfudeathgrip at the top of a method, then do
> random stuff, then use the field that was gripped, like this:
> http://dev.searchfox.org/mozilla-central/source/dom/base/TextInputProcessor.
> cpp#377

Yeah, that code is probably broken if StartComposition() can do something that can destroy mDispatcher.  Given that function dispatches an event, there is a very likely chance that there's a sec-crit forming here.  :(  (The only thing that saves us today is that IsComposing() is not virtual.)

> It seems like one has to either (a) audit all of the intermediate methods to
> see if they change the mFoo method, or (b) move the grip down to where the
> method is actually called on the field. (I could even imagine some helper
> class that does the grip, the method call, then immediately clears the
> pointer to avoid abuse.)  (b) sounds better to me, but of course other code
> might implicitly depend on the earlier grip.
> 
> Ehsan, do you have any thoughts on this?

(b) won't help here at all since the thing that will UAF is the call to IsComposing() (if it were virtual).
(a) won't help either since even if StartComposition() doesn't touch mFoo, it can do stuff that can very indirectly change mFoo (such as deleting the object pointed to by |this| and so on.)

The fix for this (and possibly all other similar cases) is to call methods on the death grip, not the member variable.

Basically, something like this:

* The static analysis looks for a death grip being constructed on the stack.
* Once such a grip is found, the variable used to construct it can only be used once after that in the function as a non-l-value.  Error out if more than once such usage is found.
* To fix the error, switch such places to use the grip rather than the member variable.

Michael, are you interested in working on this?

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to :Ehsan Akhgari from comment #10)
> (b) won't help here at all since the thing that will UAF is the call to
> IsComposing() (if it were virtual).

How is that? You mean that |this| could go away? That feels to me more like insufficient death gripping of |this| rather than of |mFoo|. For instance, in the code I linked above, we call |MaybeDispatchKeyupForComposition()| on |this| after |StartComposition|.

> (a) won't help either since even if StartComposition() doesn't touch mFoo,
> it can do stuff that can very indirectly change mFoo (such as deleting the
> object pointed to by |this| and so on.)

That's true, it would be very hard to audit in general. In the handful of cases I looked at so far, the methods in the middle didn't seem to write anything so it seems okay. Of course, this is fragile.

Comment 12

[(no longer active)]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> (In reply to :Ehsan Akhgari from comment #10)
> > (b) won't help here at all since the thing that will UAF is the call to
> > IsComposing() (if it were virtual).
> 
> How is that? You mean that |this| could go away? That feels to me more like
> insufficient death gripping of |this| rather than of |mFoo|. For instance,
> in the code I linked above, we call |MaybeDispatchKeyupForComposition()| on
> |this| after |StartComposition|.

No, mDispatcher may end up pointing to a second object (for example through an assignment on mDispatcher) while the death grip still keeps pointing to the original object, and then the second object may die normally without the death grip helping to keep it alive.

(Not saying that this code may not have other insufficient gripping problems in it...)

> > (a) won't help either since even if StartComposition() doesn't touch mFoo,
> > it can do stuff that can very indirectly change mFoo (such as deleting the
> > object pointed to by |this| and so on.)
> 
> That's true, it would be very hard to audit in general. In the handful of
> cases I looked at so far, the methods in the middle didn't seem to write
> anything so it seems okay. Of course, this is fragile.

Michael and I actually came up with a solution which seems like it's the right thing to do:

Prevent construction of death grips from anything other than the this pointer unless if the death grips are used to access the thing that they're constructed from, rather than the variable pointing to that thing.  That should hopefully reduce the need for auditing this stuff manually.

The only part that sucks about it is that there is no generic way to detect death grips.  So far, an unused nsCOMPtr/RefPtr on the stack seems to fit the bill...  If you have any better ideas, please let us know!

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to :Ehsan Akhgari from comment #12)
> No, mDispatcher may end up pointing to a second object (for example through
> an assignment on mDispatcher) while the death grip still keeps pointing to
> the original object, and then the second object may die normally without the
> death grip helping to keep it alive.

Oh, I see, you meant the deathgrip fails to protect the second call. Right.

> Prevent construction of death grips from anything other than the this
> pointer unless if the death grips are used to access the thing that they're
> constructed from, rather than the variable pointing to that thing.  That
> should hopefully reduce the need for auditing this stuff manually.

Sounds reasonable.

> So far, an unused nsCOMPtr/RefPtr on the stack seems to fit
> the bill...  If you have any better ideas, please let us know!

Yeah, that should cover most of them. Though if you search for "kungFuDeathGrip", some of them have uses! Maybe you could also check for variables named kungFuDeathGrip? ;)

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: List of matched nsCOMPtrs

This is my current list of nsCOMPtrs and RefPtrs which are being matched by my analysis (which I haven't looked at yet). 

49 of them are actually just unused uninitialized pointers, while the rest need to be looked at and marked with either 

mozilla::Unused << value;

or changed to use the pointer.

I'll post the analysis soonish hopefully.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Changes in dom/

Comment 17

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Changes in editor/

This is one of the patches I'm most worried about, mostly due to its size and the number of changes I have made to it.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Changes in widget/cocoa/

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Changes in layout/

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Changes in docshell/ and uriloader/

Comment 21

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Changes in widget/gtk/

Comment 22

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Various other changes

Comment 23

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 10: Disable the kungFuDeathGrip analysis on optimized builds

Comment 24

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Changes in parser/

Comment 25

[Markus Stange [:mstange]]

Comment on attachment 8772101 [details] [diff] [review]
Part 4: Changes in widget/cocoa/

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

Some of these seem quite unnecessary, but this patch doesn't make anything worse.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8772103 [details] [diff] [review]
Part 6: Changes in docshell/ and uriloader/

>+  mozilla::Unused << loadingSHE; // XXX: Not sure if we need this anymore

I'm not sure we do.  The one consumer seems to be gone now...

r=me either way, but maybe file a followup on nixing this bit?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8772102 [details] [diff] [review]
Part 5: Changes in layout/

Why is the change in layout/xul/nsXULPopupManager.cpp ok?

r=me on the rest, but that change needs either explanation or undoing.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8772109 [details] [diff] [review]
Part 9: Various other changes

> nsWebBrowser::SetDocShell(nsIDocShell* aDocShell)
> {
>-  nsCOMPtr<nsIDocShell> kungFuDeathGrip(mDocShell);
>+  nsCOMPtr<nsIDocShell> docShell(mDocShell);
>   if (aDocShell) {
>-    NS_ENSURE_TRUE(!mDocShell, NS_ERROR_FAILURE);
>+    NS_ENSURE_TRUE(!docShell, NS_ERROR_FAILURE);

kungFuDeathGrip is perfectly valid here.
Other option is to move nsCOMPtr<nsIDocShell> to 'else' and somehow use it there, but it would be still a bit messy, given what kinds of member variables there.
So, I'd just keep kungFuDeathGrip.


>+++ b/security/manager/pki/nsNSSDialogs.cpp
>@@ -259,19 +259,16 @@ nsNSSDialogs::PickCertificate(nsIInterfaceRequestor *ctx,
>                               int32_t *selectedIndex, 
>                               bool *canceled) 
> {
>   nsresult rv;
>   uint32_t i;
> 
>   *canceled = false;
> 
>-  // Get the parent window for the dialog
>-  nsCOMPtr<nsIDOMWindow> parent = do_GetInterface(ctx);
>-
This looks possibly regression risky. We explicitly keep the parent (assuming it is non-null) alive while showing the dialog (and spinning the event loop), even though we don't 
actually use the parent for anything. But probably fine. Based on the blame this looks like some ancient leftover.

> nsNSSDialogs::ChooseToken(nsIInterfaceRequestor *aCtx, const char16_t **aTokenList, uint32_t aCount, char16_t **aTokenChosen, bool *aCanceled) {
>   nsresult rv;
>   uint32_t i;
> 
>   *aCanceled = false;
> 
>-  // Get the parent window for the dialog
>-  nsCOMPtr<nsIDOMWindow> parent = do_GetInterface(aCtx);
This looks similar. But ok to me.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8772108 [details] [diff] [review]
Part 8: Changes in widget/gtk/

If you do this, you need to create new kungFuDeathGrip in DispatchCompositionStart() because it compares old mLastFocusedWindow and *current* mLastFocusedWindow after a call of |dispatcher->StartComposition(status)| which dispatches eCompositionStart event.

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8772097 [details] [diff] [review]
Part 2: Changes in dom/

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

Great! This patch is 3 or 4 patches in 1. Everything is OK, but I want 3 or 4 patches and not 1 huge page.
Split it, file bugs, etc etc.

r- just because of this.

::: dom/animation/Animation.cpp
@@ +158,5 @@
>  // https://w3c.github.io/web-animations/#setting-the-timeline
>  void
>  Animation::SetTimelineNoUpdate(AnimationTimeline* aTimeline)
>  {
>    RefPtr<AnimationTimeline> oldTimeline = mTimeline;

move this line to line 166.

@@ +159,5 @@
>  void
>  Animation::SetTimelineNoUpdate(AnimationTimeline* aTimeline)
>  {
>    RefPtr<AnimationTimeline> oldTimeline = mTimeline;
> +  if (oldTimeline == aTimeline) {

and keep mTimeline here.

::: dom/base/TextInputProcessor.cpp
@@ +74,5 @@
>      // If this is composing and not canceling the composition, nobody can steal
>      // the rights of TextEventDispatcher from this instance.  Therefore, this
>      // needs to cancel the composition here.
>      if (NS_SUCCEEDED(IsValidStateForComposition())) {
> +      RefPtr<TextEventDispatcher> dispatcher(mDispatcher);

I like the name kungFuDeathGrib... why renaming it? Add a comment saying that we keep a reference to keep the object alive.

@@ +373,5 @@
>    MOZ_RELEASE_ASSERT(aSucceeded, "aSucceeded must not be nullptr");
>    MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome());
>    *aSucceeded = false;
>  
> +  RefPtr<TextEventDispatcher> dispatcher(mDispatcher);

kungfuDeathGrip is a magic world in mozilla codebase. If you remove it, add a comment saying that we keep a ref to keep the object alive.

@@ +409,5 @@
>  NS_IMETHODIMP
>  TextInputProcessor::SetPendingCompositionString(const nsAString& aString)
>  {
>    MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome());
> +  RefPtr<TextEventDispatcher> dispatcher(mDispatcher);

here as well.

@@ +422,5 @@
>  TextInputProcessor::AppendClauseToPendingComposition(uint32_t aLength,
>                                                       uint32_t aAttribute)
>  {
>    MOZ_RELEASE_ASSERT(nsContentUtils::IsCallerChrome());
> +  RefPtr<TextEventDispatcher> dispatcher(mDispatcher);

ok.. easy: apply the comment _everywhere_ :)

::: dom/base/nsDocument.cpp
@@ -4881,5 @@
>    NS_ASSERTION(!mScriptGlobalObject ||
>                 mScriptGlobalObject == aScriptObject,
>                 "Wrong script object!");
> -  // XXXkhuey why bother?
> -  nsCOMPtr<nsPIDOMWindowInner> win = do_QueryInterface(aScriptObject);

This is completely unrelated, right?

@@ -7783,5 @@
>    if (aNode->IsElement()) {
>      Element *element = aNode->AsElement();
>      const nsDOMAttributeMap *map = element->GetAttributeMap();
>      if (map) {
> -      nsCOMPtr<nsIAttribute> attr;

unrelated. File a separate bug to remove variables. This patch is already quite big.

@@ -8831,5 @@
>    // in a form, and reset the password and autocomplete=off elements.
>  
>    RefPtr<nsContentList> nodes = GetElementsByTagName(NS_LITERAL_STRING("input"));
>  
> -  nsCOMPtr<nsIContent> item;

ditto.

@@ -10499,5 @@
>    //
>    // mStateObjectContainer may be null; this just means that there's no
>    // current state object.
>  
> -  nsCOMPtr<nsIVariant> stateObj;

here as well.

@@ -10940,5 @@
>    bool nodeIsAnonymous = node && node->IsInNativeAnonymousSubtree();
>    if (nodeIsAnonymous) {
>      node = ptFrame->GetContent();
>      nsIContent* nonanon = node->FindFirstNonChromeOnlyAccessContent();
> -    nsCOMPtr<nsIDOMHTMLInputElement> input = do_QueryInterface(nonanon);

here too.

::: dom/base/nsDocumentEncoder.cpp
@@ -1120,5 @@
>  
>    nsresult rv = NS_OK;
>  
> -  nsCOMPtr<nsIAtom> charsetAtom;
> -  

separate patch. All these removes can happen in the same patch. but not this one.

@@ -1650,5 @@
>    
>    nsCOMPtr<nsIDOMNode> opStartNode;
>    nsCOMPtr<nsIDOMNode> opEndNode;
>    int32_t opStartOffset, opEndOffset;
> -  nsCOMPtr<nsIDOMRange> opRange;

just because you are here, can you remove all this non-sense extra spaces from line 1650 to line 1661 ?

::: dom/base/nsFrameLoader.cpp
@@ +2696,5 @@
>    nsFrameLoader* dest = static_cast<nsFrameLoader*>(aDest);
>    dest->MaybeCreateDocShell();
>    NS_ENSURE_STATE(dest->mDocShell);
>  
> +  dest->mDocShell->GetDocument();

revert this changes.

::: dom/base/nsFrameMessageManager.cpp
@@ +1307,5 @@
>        }
>      }
>    }
> +  RefPtr<nsFrameMessageManager> parentManager = mParentManager;
> +  return parentManager ? parentManager->ReceiveMessage(aTarget, aTargetFrameLoader,

ok. so... what's the reason to have an extra reference here?
Probably you can just remove line RefPtr<nsFrameMessageManager> kungfuDeathGrip = mParentManager.
Right?

::: dom/base/nsGlobalWindow.cpp
@@ +454,5 @@
>      if (!mInnerWindow) {                                                      \
>        if (mIsClosed) {                                                        \
>          return err_rval;                                                      \
>        }                                                                       \
> +      GetDoc();                                                               \

Revert this changes. What about if these getters return a raw pointer? They don't... but they could :)

@@ +3690,5 @@
>    if (nsIDocShell* docShell = GetDocShell()) {
>      // Note that |document| here is the same thing as our mDoc, but we
>      // don't have to explicitly set the member variable because the docshell
>      // has already called SetNewDocument().
> +    docShell->GetDocument();

ditto.

@@ +11881,5 @@
>        // if there is no document in the window.
>        // XXXbz should this just use EnsureInnerWindow()?
>  
>        // Force document creation.
> +      (*aReturn)->GetDoc();

here as well. What I propose here is this:

1. separate patch :)
2. expose a MaybeCreateDoc() that doesn't assert !mDoc.
3. call it here, and everywhere else we do GetDoc().

follow up. separate patch and separate bug. Thanks :)

@@ -14013,5 @@
>  nsGlobalModalWindow::GetReturnValue(nsIVariant **aRetVal)
>  {
>    FORWARD_TO_OUTER_MODAL_CONTENT_WINDOW(GetReturnValue, (aRetVal), NS_OK);
>  
> -  nsCOMPtr<nsIVariant> result;

This goes in patch 2.

::: dom/base/nsHostObjectProtocolHandler.cpp
@@ -93,5 @@
>            "A blob URL allocated with URL.createObjectURL; the referenced "
>            "blob cannot be freed until all URLs for it have been explicitly "
>            "invalidated with URL.revokeObjectURL.");
>          nsAutoCString path, url, owner, specialDesc;
> -        nsCOMPtr<nsIURI> principalURI;

patch 2.

::: dom/base/nsLocation.cpp
@@ -250,5 @@
>  {
>    nsCOMPtr<nsIDocShell> docShell(do_QueryReferent(mDocShell));
>    if (docShell) {
>      nsCOMPtr<nsIDocShellLoadInfo> loadInfo;
> -    nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(docShell));

patch 2 as well.

::: dom/base/nsScriptLoader.cpp
@@ -2514,5 @@
>      if (!nsContentUtils::IsJavascriptMIMEType(typeString)) {
>        return NS_ERROR_FAILURE;
>      }
>  
> -    nsCOMPtr<nsIURI> baseURL;

guess :)

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +1356,5 @@
>  
>    RefPtr<SourceSurface> snapshot;
>    Matrix transform;
>    RefPtr<PersistentBufferProvider> oldBufferProvider = mBufferProvider;
> +  mozilla::Unused << oldBufferProvider; // Not sure if this is necessary

call it kungFuDeathGrip

::: dom/devicestorage/nsDeviceStorage.cpp
@@ -3115,5 @@
>    if (!aBlob) {
>      return nullptr;
>    }
>  
> -  nsCOMPtr<nsIRunnable> r;

patch 2.

::: dom/geolocation/nsGeoPositionIPCSerialiser.h
@@ +82,5 @@
>            && ReadParam(aMsg, aIter, &speed            ))) return false;
>  
>      // We now have all the data
> +    RefPtr<nsGeoPositionCoords> coords
> +      = new nsGeoPositionCoords(latitude,         /* aLat     */

2 things:

1. it's completely unrelated change, right? Move it separately.
2. move = in the previous line.

RefPtr<nsGeoPositionCoords> coords =
  new ...

@@ +137,4 @@
>  
>      // It's not important to us where it fails, but rather if it fails
>      if (!ReadParam(aMsg, aIter, &timeStamp) ||
> +        !ReadParam(aMsg, aIter, (nsIDOMGeoPositionCoords**)getter_AddRefs(coords))) {

why this cast? It should not needed.

::: dom/html/HTMLSelectElement.cpp
@@ -613,5 @@
>      return error.StealNSResult();
>    }
>  
>    nsCOMPtr<nsISupports> supports;
> -  nsCOMPtr<nsIDOMHTMLElement> beforeElement;

patch 2.

::: dom/html/nsGenericHTMLElement.cpp
@@ -3218,5 @@
>  void
>  nsGenericHTMLElement::SetInnerText(const nsAString& aValue)
>  {
> -  // Fire DOMNodeRemoved mutation events before we do anything else.
> -  nsCOMPtr<nsIContent> kungFuDeathGrip;

patch 2.

::: dom/ipc/ContentParent.cpp
@@ -1146,5 @@
>      openerTabId = TabParent::GetTabIdFrom(docShell);
>    }
>  
>    if (aContext.IsMozBrowserElement() || !aContext.HasOwnApp()) {
> -    RefPtr<TabParent> tp;

patch 2.

::: dom/media/MediaDecoderReader.cpp
@@ -526,5 @@
>  
>    // Shut down the watch manager before shutting down our task queue.
>    mWatchManager.Shutdown();
>  
> -  RefPtr<ShutdownPromise> p;

patch 2.

::: dom/media/gmp/GMPServiceParent.cpp
@@ +879,5 @@
>  NS_IMETHODIMP
>  GeckoMediaPluginServiceParent::AddPluginDirectory(const nsAString& aDirectory)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  AsyncAddPluginDirectory(aDirectory);

don't do this. Here we leak.

::: dom/plugins/base/nsPluginStreamListenerPeer.cpp
@@ -410,5 @@
>      if (NS_FAILED(rv))
>        return rv;
>  
>      // create a file output stream to write to...
> -    nsCOMPtr<nsIOutputStream> outstream;

guess.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ -3354,5 @@
>      MOZ_ASSERT(mSurfaceType != gfxSurfaceType::Max,
>                 "Need a valid surface type here");
>      NS_ASSERTION(!mCurrentSurface, "mCurrentSurfaceActor can get out of sync.");
>  
> -    RefPtr<gfxASurface> retsurf;

here too.

@@ +3584,2 @@
>      RefPtr<gfxASurface> curSurface = mHelperSurface ? mHelperSurface : mCurrentSurface;
> +#endif

here, write a comment.

::: dom/quota/ActorsParent.cpp
@@ -7477,5 @@
>  UpgradeDirectoryMetadataFrom1To2Helper::DoProcessOriginDirectories()
>  {
>    AssertIsOnIOThread();
>  
> -  nsCOMPtr<nsIFile> permanentStorageDir;

2

::: dom/storage/DOMStorageDBThread.cpp
@@ -455,5 @@
>    nsCOMPtr<mozIStorageService> service
>        = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  nsCOMPtr<mozIStorageConnection> connection;

2

::: dom/xml/nsXMLFragmentContentSink.cpp
@@ -166,5 @@
>  
>  NS_IMETHODIMP 
>  nsXMLFragmentContentSink::DidBuildModel(bool aTerminated)
>  {
> -  RefPtr<nsParserBase> kungFuDeathGrip(mParser);

2

::: dom/xul/nsXULElement.cpp
@@ -1712,5 @@
>  void
>  nsXULElement::Focus(ErrorResult& rv)
>  {
>      nsIFocusManager* fm = nsFocusManager::GetFocusManager();
> -    nsCOMPtr<nsIDOMElement> elem = do_QueryObject(this);

2

::: dom/xul/templates/nsXULTemplateQueryProcessorStorage.cpp
@@ -224,5 @@
>              nsXULContentUtils::LogTemplateError(ERROR_TEMPLATE_STORAGE_BAD_URI);
>              return rv;
>          }
>  
> -        nsCOMPtr<nsIFile> file;

2

Comment 31

[(no longer active)]

Comment on attachment 8772098 [details] [diff] [review]
Part 3: Changes in editor/

Masayuki, can you please review this?  Thanks!

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8772098 [details] [diff] [review]
Part 3: Changes in editor/

>@@ -349,16 +349,17 @@ EditorEventListener::EditorHasFocus()
> NS_IMPL_ISUPPORTS(EditorEventListener, nsIDOMEventListener)
> 
> NS_IMETHODIMP
> EditorEventListener::HandleEvent(nsIDOMEvent* aEvent)
> {
>   NS_ENSURE_TRUE(mEditorBase, NS_ERROR_FAILURE);
> 
>   nsCOMPtr<nsIEditor> kungFuDeathGrip = mEditorBase;
>+  Unused << kungFuDeathGrip; // mEditorBase is not referred to in this function

Does this cause unused warning or something? Please make the reason of this change clear with comment.

>diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp
>@@ -292,45 +292,45 @@ NS_IMETHODIMP
> HTMLEditRules::BeforeEdit(EditAction action,
>                           nsIEditor::EDirection aDirection)
> {

>     // Register with range updater to track this as we perturb the doc
>-    (mHTMLEditor->mRangeUpdater).RegisterRangeItem(mRangeItem);
>+    (htmlEditor->mRangeUpdater).RegisterRangeItem(mRangeItem);

I don't know why the |()| is necessary here... but up to you if you try to remove it.

>@@ -374,33 +374,33 @@ NS_IMETHODIMP
> HTMLEditRules::AfterEdit(EditAction action,
>                          nsIEditor::EDirection aDirection)
> {

>     // Free up selectionState range item
>-    (mHTMLEditor->mRangeUpdater).DropRangeItem(mRangeItem);
>+    (htmlEditor->mRangeUpdater).DropRangeItem(mRangeItem);

Same here...

>@@ -1185,21 +1185,21 @@ HTMLEditRules::WillInsert(Selection& aSelection,
>   // If we are after a mozBR in the same block, then move selection to be
>   // before it
>   NS_ENSURE_TRUE_VOID(aSelection.GetRangeAt(0) &&
>                       aSelection.GetRangeAt(0)->GetStartParent());
>   OwningNonNull<nsINode> selNode = *aSelection.GetRangeAt(0)->GetStartParent();
>   int32_t selOffset = aSelection.GetRangeAt(0)->StartOffset();
> 
>   // Get prior node
>-  nsCOMPtr<nsIContent> priorNode = mHTMLEditor->GetPriorHTMLNode(selNode,
>+  nsCOMPtr<nsIContent> priorNode = htmlEditor->GetPriorHTMLNode(selNode,
>                                                                  selOffset);

nit: odd indent here.

>   // If block is empty, populate with br.  (For example, imagine a div that
>   // contains the word "text".  The user selects "text" and types return.
>   // "Text" is deleted leaving an empty block.  We want to put in one br to
>   // make block have a line.  Then code further below will put in a second br.)
>   bool isEmpty;
>   IsEmptyBlock(*blockParent, &isEmpty);
>   if (isEmpty) {
>-    nsCOMPtr<Element> br = mHTMLEditor->CreateBR(blockParent,
>+    nsCOMPtr<Element> br = htmlEditor->CreateBR(blockParent,
>                                                  blockParent->Length());

nit: same here.

>-      aOffset = mHTMLEditor->SplitNodeDeep(*linkNode, *node->AsContent(),
>+      aOffset = htmlEditor->SplitNodeDeep(*linkNode, *node->AsContent(),
>                                            aOffset,
>                                            HTMLEditor::EmptyContainers::no);

nit: same here.

>-        res = mHTMLEditor->SplitStyleAbovePoint(address_of(previousContentParent),
>+        res = htmlEditor->SplitStyleAbovePoint(address_of(previousContentParent),
>                                                 &previousContentOffset,
>                                                 nullptr, nullptr, nullptr,
>                                                 getter_AddRefs(splittedPreviousContent));

nit: same here.

>-        NS_ENSURE_STATE(mHTMLEditor);
>-        mHTMLEditor->mCSSEditUtils->ParseLength(value, &f,
>+        NS_ENSURE_STATE(htmlEditor);
>+        htmlEditor->mCSSEditUtils->ParseLength(value, &f,
>                                                 getter_AddRefs(unit));

nit: same here.

>   // Do the splits!
>   nsCOMPtr<nsIContent> newMiddleNode1;
>-  mHTMLEditor->SplitNodeDeep(aBlock, startParent, startOffset,
>+  htmlEditor->SplitNodeDeep(aBlock, startParent, startOffset,
>                              HTMLEditor::EmptyContainers::no,
>                              aOutLeftNode, getter_AddRefs(newMiddleNode1));

nit: same here.

>   // Do the splits!
>   nsCOMPtr<nsIContent> newMiddleNode2;
>-  mHTMLEditor->SplitNodeDeep(aBlock, endParent, endOffset,
>+  htmlEditor->SplitNodeDeep(aBlock, endParent, endOffset,
>                              HTMLEditor::EmptyContainers::no,
>                              getter_AddRefs(newMiddleNode2), aOutRightNode);

nit: same here.

>       // Making use of html structure... if next node after where we are
>       // putting our div is not a block, then the br we found is in same block
>       // we are, so it's safe to consume it.
>-      nsCOMPtr<nsIContent> sibling = mHTMLEditor->GetNextHTMLSibling(parent,
>+      nsCOMPtr<nsIContent> sibling = htmlEditor->GetNextHTMLSibling(parent,
>                                                                      offset);

nit: same here.


>       if (sibling && !IsBlockNode(*sibling)) {
>-        rv = mHTMLEditor->DeleteNode(brContent);
>+        rv = htmlEditor->DeleteNode(brContent);
>         NS_ENSURE_SUCCESS(rv, rv);
>       }
>     }
>-    nsCOMPtr<Element> div = mHTMLEditor->CreateNode(nsGkAtoms::div, parent,
>+    nsCOMPtr<Element> div = htmlEditor->CreateNode(nsGkAtoms::div, parent,
>                                                     offset);

nit: same here.

>     if (HTMLEditUtils::IsListItem(emptyBlock)) {
>       // Are we the first list item in the list?
>       bool bIsFirst;
>-      NS_ENSURE_STATE(mHTMLEditor);
>-      res = mHTMLEditor->IsFirstEditableChild(GetAsDOMNode(emptyBlock),
>+      NS_ENSURE_STATE(htmlEditor);
>+      res = htmlEditor->IsFirstEditableChild(GetAsDOMNode(emptyBlock),
>                                               &bIsFirst);

nit: same here.

>         // Move to the start of the next node if it's a text.
>-        nsCOMPtr<nsIContent> nextNode = mHTMLEditor->GetNextNode(blockParent,
>+        nsCOMPtr<nsIContent> nextNode = htmlEditor->GetNextNode(blockParent,
>                                                                  offset + 1, true);

nit: same here. And if it over 80 characters, I'd like you to break the line after |=|.

>         // Move to the end of the previous node if it's a text.
>-        nsCOMPtr<nsIContent> priorNode = mHTMLEditor->GetPriorNode(blockParent,
>+        nsCOMPtr<nsIContent> priorNode = htmlEditor->GetPriorNode(blockParent,
>                                                                    offset,
>                                                                    true);

nit: odd indent.

>         if (0 < offset && offset < (int32_t)(tempString.Length())) {
>           // Split the text node.
>           nsCOMPtr<nsIDOMNode> tempNode;
>-          res = mHTMLEditor->SplitNode(endParent->AsDOMNode(), offset,
>+          res = htmlEditor->SplitNode(endParent->AsDOMNode(), offset,
>                                        getter_AddRefs(tempNode));

nit: same.

>     int32_t resultOffset =
>-      mHTMLEditor->SplitNodeDeep(*splitDeepNode, splitParentNode, splitOffset,
>+      htmlEditor->SplitNodeDeep(*splitDeepNode, splitParentNode, splitOffset,
>                                  HTMLEditor::EmptyContainers::yes,
>                                  getter_AddRefs(leftNode),
>                                  getter_AddRefs(rightNode));

nit: same.

>     NS_ENSURE_STATE(resultOffset != -1);
> 
>     // Put left node in node list
>     if (leftNode) {
>       // Might not be a left node.  A break might have been at the very
>       // beginning of inline container, in which case SplitNodeDeep would not
>       // actually split anything
>       aOutArrayOfNodes.AppendElement(*leftNode);
>     }
>     // Move break outside of container and also put in node list
>-    nsresult res = mHTMLEditor->MoveNode(breakNode, inlineParentNode,
>+    nsresult res = htmlEditor->MoveNode(breakNode, inlineParentNode,
>                                          resultOffset);

nit: same.

>@@ -6734,17 +6732,17 @@ HTMLEditRules::ApplyBlockStyle(nsTArray<OwningNonNull<nsINode>>& aNodeArray,
> 
>     // If curNode is a address, p, header, address, or pre, replace it with a
>     // new block of correct type.
>     // XXX: pre can't hold everything the others can
>     if (HTMLEditUtils::IsMozDiv(curNode) ||
>         HTMLEditUtils::IsFormatNode(curNode)) {
>       // Forget any previous block used for previous inline nodes
>       curBlock = nullptr;
>-      newBlock = mHTMLEditor->ReplaceContainer(curNode->AsElement(),
>+      newBlock = htmlEditor->ReplaceContainer(curNode->AsElement(),
>                                                &aBlockTag, nullptr, nullptr,
>                                                EditorBase::eCloneAttributes);

nit: same.

>@@ -7571,17 +7569,17 @@ HTMLEditRules::RemoveEmptyNodes()
>             bIsCandidate = true;
>           }
>         }
>       }
> 
>       if (bIsCandidate) {
>         // We delete mailcites even if they have a solo br in them.  Other
>         // nodes we require to be empty.
>-        res = mHTMLEditor->IsEmptyNode(node->AsDOMNode(), &bIsEmptyNode,
>+        res = htmlEditor->IsEmptyNode(node->AsDOMNode(), &bIsEmptyNode,
>                                        bIsMailCite, true);

nit: same.

>@@ -8357,60 +8355,60 @@ HTMLEditRules::AlignBlock(Element& aElement,
>                           ContentsOnly aContentsOnly)
> {
>   if (!IsBlockNode(aElement) && !aElement.IsHTMLElement(nsGkAtoms::hr)) {
>     // We deal only with blocks; early way out
>     return NS_OK;
>   }
> 
>   NS_ENSURE_STATE(mHTMLEditor);
>-  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);
>+  RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
> 
>   nsresult res = RemoveAlignment(aElement.AsDOMNode(), aAlignType,
>                                  aContentsOnly == ContentsOnly::yes);
>   NS_ENSURE_SUCCESS(res, res);
>   NS_NAMED_LITERAL_STRING(attr, "align");
>-  if (mHTMLEditor->IsCSSEnabled()) {
>+  if (htmlEditor->IsCSSEnabled()) {
>     // Let's use CSS alignment; we use margin-left and margin-right for tables
>     // and text-align for other block-level elements
>-    res = mHTMLEditor->SetAttributeOrEquivalent(
>+    res = htmlEditor->SetAttributeOrEquivalent(
>       static_cast<nsIDOMElement*>(aElement.AsDOMNode()), attr, aAlignType,
>       false);
>     NS_ENSURE_SUCCESS(res, res);
>   } else {
>     // HTML case; this code is supposed to be called ONLY if the element
>     // supports the align attribute but we'll never know...
>     if (HTMLEditUtils::SupportsAlignAttr(aElement.AsDOMNode())) {
>       res =
>-        mHTMLEditor->SetAttribute(static_cast<nsIDOMElement*>(aElement.AsDOMNode()),
>+        htmlEditor->SetAttribute(static_cast<nsIDOMElement*>(aElement.AsDOMNode()),
>                                   attr, aAlignType);

nit: same.

> nsresult
> HTMLEditRules::ChangeIndentation(Element& aElement,
>                                  Change aChange)
> {
>   NS_ENSURE_STATE(mHTMLEditor);
>-  nsCOMPtr<nsIEditor> kungFuDeathGrip(mHTMLEditor);
>+  RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
> 
>   nsIAtom& marginProperty =
>-    MarginPropertyAtomForIndent(*mHTMLEditor->mCSSEditUtils, aElement);
>+    MarginPropertyAtomForIndent(*htmlEditor->mCSSEditUtils, aElement);
>   nsAutoString value;
>-  mHTMLEditor->mCSSEditUtils->GetSpecifiedProperty(aElement, marginProperty,
>+  htmlEditor->mCSSEditUtils->GetSpecifiedProperty(aElement, marginProperty,
>                                                    value);

Same.

>@@ -8428,62 +8426,62 @@ HTMLEditRules::ChangeIndentation(Element& aElement,
>   } else if (nsGkAtoms::percentage == unit) {
>             f += NS_EDITOR_INDENT_INCREMENT_PERCENT * multiplier;
>   }
> 
>   if (0 < f) {
>     nsAutoString newValue;
>     newValue.AppendFloat(f);
>     newValue.Append(nsDependentAtomString(unit));
>-    mHTMLEditor->mCSSEditUtils->SetCSSProperty(aElement, marginProperty,
>+    htmlEditor->mCSSEditUtils->SetCSSProperty(aElement, marginProperty,
>                                                newValue);

nit: same.

>     return NS_OK;
>   }
> 
>-  mHTMLEditor->mCSSEditUtils->RemoveCSSProperty(aElement, marginProperty,
>+  htmlEditor->mCSSEditUtils->RemoveCSSProperty(aElement, marginProperty,
>                                                 value);

nit: same.
>@@ -8532,54 +8530,54 @@ HTMLEditRules::WillAbsolutePosition(Selection& aSelection,
>       if (!curList || (sibling && sibling != curList)) {
>         // Create a new nested list of correct type
>         res = SplitAsNeeded(*curParent->NodeInfo()->NameAtom(), curParent,
>                             offset);
>         NS_ENSURE_SUCCESS(res, res);
>         if (!curPositionedDiv) {
>           nsCOMPtr<nsINode> curParentParent = curParent->GetParentNode();
>           int32_t parentOffset = curParentParent
>             ? curParentParent->IndexOf(curParent) : -1;
>-          curPositionedDiv = mHTMLEditor->CreateNode(nsGkAtoms::div, curParentParent,
>+          curPositionedDiv = htmlEditor->CreateNode(nsGkAtoms::div, curParentParent,
>                                                      parentOffset);

nit: same.

>           mNewBlock = curPositionedDiv;
>         }
>-        curList = mHTMLEditor->CreateNode(curParent->NodeInfo()->NameAtom(),
>+        curList = htmlEditor->CreateNode(curParent->NodeInfo()->NameAtom(),
>                                           curPositionedDiv, -1);

nit: same.

>@@ -8589,63 +8587,63 @@ HTMLEditRules::WillAbsolutePosition(Selection& aSelection,
>-          curList = mHTMLEditor->CreateNode(curParent->NodeInfo()->NameAtom(),
>+          curList = htmlEditor->CreateNode(curParent->NodeInfo()->NameAtom(),
>                                             curPositionedDiv, -1);

nit: same.

>diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp
> NS_IMETHODIMP
> HTMLEditor::StartOperation(EditAction opID,
>                            nsIEditor::EDirection aDirection)
> {
>   // Protect the edit rules object from dying
>-  nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules);
>+  nsCOMPtr<nsIEditRules> rules(mRules);
> 
>   EditorBase::StartOperation(opID, aDirection);  // will set mAction, mDirection
>-  if (mRules) return mRules->BeforeEdit(mAction, mDirection);
>+  if (rules) return rules->BeforeEdit(mAction, mDirection);

Could you rewrite this as:

if (rules) {
  return rules->BeforeEdit(mAction, mDirection);
}

?

>   // post processing
>   nsresult res = NS_OK;
>-  if (mRules) res = mRules->AfterEdit(mAction, mDirection);
>+  if (rules) res = rules->AfterEdit(mAction, mDirection);

Same.

>@@ -1545,51 +1545,51 @@ TextEditor::GetEmbeddedObjects(nsISupportsArray** aNodeList)
>  * All editor operations which alter the doc should be prefaced
>  * with a call to StartOperation, naming the action and direction.
>  */
> NS_IMETHODIMP
> TextEditor::StartOperation(EditAction opID,
>                            nsIEditor::EDirection aDirection)
> {
>   // Protect the edit rules object from dying
>-  nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules);
>+  nsCOMPtr<nsIEditRules> rules(mRules);
> 
>   EditorBase::StartOperation(opID, aDirection);  // will set mAction, mDirection
>-  if (mRules) return mRules->BeforeEdit(mAction, mDirection);
>+  if (rules) return rules->BeforeEdit(mAction, mDirection);

Same.

> NS_IMETHODIMP
> TextEditor::EndOperation()
> {
>   // Protect the edit rules object from dying
>-  nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules);
>+  nsCOMPtr<nsIEditRules> rules(mRules);
> 
>   // post processing
>   nsresult res = NS_OK;
>-  if (mRules) res = mRules->AfterEdit(mAction, mDirection);
>+  if (rules) res = rules->AfterEdit(mAction, mDirection);

Same.

> nsresult
> TextEditor::SelectEntireDocument(Selection* aSelection)
> {
>   if (!aSelection || !mRules) { return NS_ERROR_NULL_POINTER; }
> 
>   // Protect the edit rules object from dying
>-  nsCOMPtr<nsIEditRules> kungFuDeathGrip(mRules);
>+  nsCOMPtr<nsIEditRules> rules(mRules);
> 
>   // is doc empty?
>   bool bDocIsEmpty;
>-  if (NS_SUCCEEDED(mRules->DocumentIsEmpty(&bDocIsEmpty)) && bDocIsEmpty)
>+  if (NS_SUCCEEDED(rules->DocumentIsEmpty(&bDocIsEmpty)) && bDocIsEmpty)
>   {

Could you move |{| to the end of the line for if condition?




I just reviewed roughly this big patch because:
* I assume that you just do:
 * Grab member variable like mHTMLEditor, mRules with RefPtr and use it in *all* lines in the method.
 * Remove unused variables.
 * So, you don't touch the logic of editor code.
* I don't have much time to check if there is following points:
 * Checking if the grabbed member variable is nullptr (but this must be safe because I don't find you replaced such points unexpectedly).
 * Checking if you forget to replace some lines between hunks (if you used MozReview, I could do that)

With those assumes and fix the above nits, r=masayuki.

Thank you for your great work!

Comment 33

[Peter Van der Beken [:peterv]]

Comment on attachment 8772113 [details] [diff] [review]
Part 7: Changes in parser/

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

Redirecting the review for the nsHtml5* files to Henri, this is really his part of the code and I want to make sure he's ok with the changes.

::: parser/htmlparser/nsParser.cpp
@@ +1037,5 @@
>    // is reenabled. To make sure that we're not deleted across
>    // the reenabling process, hold a reference to ourselves.
>    nsresult result=NS_OK;
>    nsCOMPtr<nsIParser> kungFuDeathGrip(this);
> +  nsCOMPtr<nsIContentSink> sink(mSink);

To be honest, I don't understand why we would rename this. The name indicates that this variable is needed to hold the thing alive, if you remove that then you need at least a comment to explain that the local variable shouldn't be removed because we need to keep the thing alive.

@@ +1833,5 @@
>      // non-whitespace data
>      if (IsOkToProcessNetworkData() &&
>          theContext->mScanner->FirstNonWhitespacePosition() >= 0) {
>        nsCOMPtr<nsIParser> kungFuDeathGrip(this);
> +      nsCOMPtr<nsIContentSink> sink(mSink);

Same here.

Comment 34

[Henri Sivonen (:hsivonen)]

Comment on attachment 8772113 [details] [diff] [review]
Part 7: Changes in parser/

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

r+ provided that the comments below are addressed. Specifically, line 359 of nsHtml5TreeOpExecutor.cpp needs to be reverted back to checking mParser.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +349,5 @@
>    }
>    
>    nsHtml5FlushLoopGuard guard(this); // this is also the self-kungfu!
>    
> +  RefPtr<nsParserBase> parser(mParser);

Please keep this named parserKungFuDeathGrip to indicate that we have the local for kung fu death grip purposes.

@@ +355,5 @@
>    // Remember the entry time
>    (void) nsContentSink::WillParseImpl();
>  
>    for (;;) {
> +    if (!parser) {

Here we really need to check mParser itself for null on each iteration.

@@ +365,5 @@
>      if (NS_FAILED(IsBroken())) {
>        return;
>      }
>  
> +    if (!parser->IsParserEnabled()) {

If we come here, we haven't done anything complex since mParser was checked for null, so we don't really need to call through the death grip.

Comment 35

[Al Billings [:abillings - ex-MoCo]]

When are the plussed patches here (7 of them) going to be checked in soon?

Comment 36

[Nika Layzell [:nika] (ni? for response)]

(In reply to Al Billings [:abillings] from comment #35)
> When are the plussed patches here (7 of them) going to be checked in soon?

Hey, sorry. Fixing up all of these patches is going to be a decent amount of tedious work which I simply haven't gotten around to doing yet. I'm hoping to get fixed up versions of all of these patches before the end of the week.

The static analysis components haven't landed yet, but the other patches should be able to land before they do.

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi Michael, we are heading into beta 7 at this point so very late to do such a risky uplift. 
Dan, is this really sec critical?

Comment 38

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Changes in dom/

MozReview-Commit-ID: 4tCUM4KRe81

Comment 39

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Changes in editor/

MozReview-Commit-ID: JA7UCVXEd8j

Comment 40

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Changes in widget/cocoa/

MozReview-Commit-ID: DhvanRhe9XE

Comment 41

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Changes in layout/

MozReview-Commit-ID: BsaKGHsoqOq

Comment 42

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Changes in docshell/ and uriloader/

MozReview-Commit-ID: GiyHWL3aaOv

Comment 43

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Changes in parser/

MozReview-Commit-ID: EN2yZUn8xj

Comment 44

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Changes in widget/gtk/

MozReview-Commit-ID: 2uJ9flIaCEY

Comment 45

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Various other changes

MozReview-Commit-ID: B0dsomkWgEk

Comment 46

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

MozReview-Commit-ID: K8rehjAxIA6

Comment 47

[Nika Layzell [:nika] (ni? for response)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> Hi Michael, we are heading into beta 7 at this point so very late to do such
> a risky uplift. 
> Dan, is this really sec critical?

I'm sorry for how long it's taken to get these patches updated. As not all of them had received review yet, I didn't feel a ton of push to perform the revisions, and they're a bit tedious to do.

I've pushed the updated versions and hopefully they should be reviewed soon. I don't think that this needs to make it into beta though. I'm not sure if it needs to be uplifted into aurora either if we don't find any vulnerabilities which this fixes. I'm not sure that it really is sec-critical (as far as I can tell, the stuff I have caught with the static analysis mostly doesn't trigger sec-critical bugs, but I may have accidentally prevented one which I didn't even notice).

Comment 48

[Daniel Veditz [:dveditz]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> Dan, is this really sec critical?

I'm reasonably confident at least one of these places could be. Since we don't know which ones, or how to trigger one, we can call it sec-high if you like. Either way this is not the kind of patch we'd want to uplift into beta. I'd like to get it into Aurora 50 if we can, though.

Comment 49

[(no longer active)]

Comment on attachment 8784554 [details] [diff] [review]
Part 9: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

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

Thanks a lot, this is great overall!

Minusing for three reasons:

* The DEBUG-only-ness
* The whitelist
* The error messages needing some improvement

::: build/clang-plugin/clang-plugin.cpp
@@ +347,5 @@
> +  return s;
> +}
> +
> +const Expr *IgnoreImplicit(const Expr *e) {
> +  return cast<Expr>(IgnoreImplicit((const Stmt *)e));

Nit: please use static_cast instead of the C-style cast.

@@ +1183,5 @@
> +#ifdef DEBUG
> +  // This analysis has too many false positives on release builds due to RefPtrs
> +  // being used as temporaries for MOZ_ASSERT etc. expressions which are
> +  // compiled out of opt builds. To avoid this, the analysis is not run on
> +  // optimized builds.

We should definitely run this analysis on the build configuration that we ship!

@@ +1742,5 @@
> +
> +  /*
> +  "The `kungFuDeathGrip` pattern is unsafe on member variables and function return values";
> +  "To disable this error, either explicitly mark the value as mozilla::Unused, or use the"
> +    " %0 to interact with this type in the future, rather than the member variable itself"

Instead of explaining things more in the comment here, I think we should improve the diagnostics.  I think the ErrorID is fine, but please also add a note saying something on the lines of "Please switch all accesses to %0 to go through %1" with arg 0 being the argument to the ctor and arg 1 being D.

@@ +1764,5 @@
> +  if (D->hasInit()) {
> +    const Expr *E = IgnoreImplicit(D->getInit());
> +    const CXXConstructExpr *CE = dyn_cast<CXXConstructExpr>(E);
> +    if (CE && CE->getNumArgs() == 0) {
> +      Diag.Report(D->getLocStart(), UninitErrorID) << D->getType()->getAsTagDecl()->getName();

I think it's much better to just not reject this case at all.  If we ever want to make an analysis that rejects code like this, it'd be much better to do it in a way that's not specific to RefPtr and nsCOMPtr.  (Also if we wanted to build such a check, we should probably implement it as a clang-tidy plugin with fix-it hints allowing you to fix the code base too.)

@@ +1769,5 @@
> +      return;
> +    }
> +
> +    while ((CE = dyn_cast<CXXConstructExpr>(E)) && CE->getNumArgs() == 1) {
> +      // Skip through unnecessary constructors

Please make this comment explain more about what's going on here.

@@ +1775,5 @@
> +    }
> +
> +
> +    // already_AddRefed and do_GetInterface can have side effects which we want
> +    // to preserve, so they are OKAY.

From this list, already_AddRefed, nsGetService* and nsCreateIntance* are the only things that people don't make kung-fu death grips out of.  So I think you'd need to remove "nsGetInterface" and "nsQueryInterfaceWithError" from the whitelist here.

Also, this can use some better comments explaining what's going on.

@@ +1799,5 @@
> +      }
> +    }
> +
> +    if (isa<CXXThisExpr>(E) || isa<DeclRefExpr>(E) || isa<CXXNewExpr>(E)) {
> +      return;

It's better to check for these cases closer to the start of the loop body to avoid running more checks than we need to.

@@ +1803,5 @@
> +      return;
> +    }
> +  }
> +
> +  // XXX: Report errors with no initializers differently.

I think this comment can be removed.

@@ +1806,5 @@
> +
> +  // XXX: Report errors with no initializers differently.
> +
> +  // Otherwise, we have an error on our hands!
> +  Diag.Report(D->getLocStart(), ErrorID) << D->getType()->getAsTagDecl()->getName();

Nit: Why not just pass D->getType()?

Comment 50

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Changes to account for modifications to clang plugin

MozReview-Commit-ID: EPQMbfHYxUK

Comment 51

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 10: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

MozReview-Commit-ID: K8rehjAxIA6

Comment 52

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Changes to account for modifications to clang plugin

Oops accidentally included some test code - fixed

Comment 53

[(no longer active)]

Comment on attachment 8786367 [details] [diff] [review]
Part 9: Changes to account for modifications to clang plugin

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

::: dom/base/nsCopySupport.cpp
@@ +545,5 @@
>    // Note that XHTML is not counted as HTML here, because we can't copy it
>    // properly (all the copy code for non-plaintext assumes using HTML
>    // serializers and parsers is OK, and those mess up XHTML).
> +  DebugOnly<nsCOMPtr<nsIHTMLDocument>> htmlDoc =
> +    nsCOMPtr<nsIHTMLDocument>(do_QueryInterface(document, &rv));

Hmm, this variable isn't used in debug builds...  I think a better way to rewrite this code is as follows:

nsCOMPtr<nsIHTMLDocument> htmlDoc = do_QueryInterface(document);
NS_ENSURE_TRUE(htmlDoc, NS_OK);

::: dom/jsurl/nsJSProtocolHandler.cpp
@@ +220,1 @@
>      if (NS_FAILED(rv)) {

Same as above.

Please rewrite by eliminating the usage of rv.

Comment 54

[(no longer active)]

Comment on attachment 8786366 [details] [diff] [review]
Part 10: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

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

Thanks, looks great!  r=me with the below nits fixed.

::: build/clang-plugin/clang-plugin.cpp
@@ +1182,5 @@
> +
> +  // This analysis has too many false positives on release builds due to RefPtrs
> +  // being used as temporaries for MOZ_ASSERT etc. expressions which are
> +  // compiled out of opt builds. To avoid this, the analysis is not run on
> +  // optimized builds.

Please eliminate the stale comment.

@@ +1757,5 @@
> +    // nsRefPtr with no arguments. This is obviously incorrect, but cannot
> +    // cause any security issues and will be easily optimized out. Until we
> +    // get a better story for running the clang-plugin locally on every build,
> +    // we will avoid breaking the build on issues like this which are only
> +    // cosmetic.

Ouch, we shouldn't be talking about security issues in the comments here.  Can you please rewrite the comment and disguise the security implications?

(Note that this patch, when landed on central, can in theory point people to investigating security issues on branches...)

Comment 55

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Changes in dom/, r=baku

MozReview-Commit-ID: 4tCUM4KRe81

Comment 56

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Changes in editor/, r=masayuki

MozReview-Commit-ID: JA7UCVXEd8j

Comment 57

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Changes in widget/cocoa/, r=mstange

MozReview-Commit-ID: DhvanRhe9XE

Comment 58

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4: Changes in layout/, r=bz

MozReview-Commit-ID: BsaKGHsoqOq

Comment 59

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5: Changes in docshell/ and uriloader/, r=bz

MozReview-Commit-ID: GiyHWL3aaOv

Comment 60

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6: Changes in parser/, r=peterv, r=hsivonen

MozReview-Commit-ID: EN2yZUn8xj

Comment 61

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7: Changes in widget/gtk/, r=masayuki

MozReview-Commit-ID: 2uJ9flIaCEY

Comment 62

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Various other changes, r=smaug

MozReview-Commit-ID: B0dsomkWgEk

Comment 63

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Changes to account for modifications to clang plugin, r=ehsan

MozReview-Commit-ID: EPQMbfHYxUK

Comment 64

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 10: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables, r=ehsan

MozReview-Commit-ID: K8rehjAxIA6

Comment 65

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8: Various other changes

MozReview-Commit-ID: B0dsomkWgEk

Comment 66

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9: Changes to account for modifications to clang plugin

MozReview-Commit-ID: EPQMbfHYxUK

Comment 67

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8788483 [details] [diff] [review]
Part 1: Changes in dom/, r=baku

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This patch potentially fixed sec critical bugs, but it changed a lot of code, and it would take effort to dig through all of the changes to find the bugs which were fixed.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
It describes a general form of the problem which we are preventing in bulk, but does not point out any specific problem.

Which older supported branches are affected by this flaw?
All of them

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No, backports would be tedious to implement, but it may be possible if necessary.

How likely is this patch to cause regressions; how much testing does it need? This patch is unlikely to cause regressions because it simply changes from putting references through a heap allocated RefPtr to a stack allocated one.

Comment 68

[Daniel Veditz [:dveditz]]

I think these changes will have to ride the trains into ESR 52 and not be back-ported to ESR-45.

Comment 69

[Daniel Veditz [:dveditz]]

If you land this before the merge next week we'll have to keep an eye on regressions and make sure those get uplifted.

Comment 70

[Nika Layzell [:nika] (ni? for response)]

pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=5399738f71c0

Comment 71

[Nika Layzell [:nika] (ni? for response)]

https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=77940cbf0c2a9f52c209fbbde5b2e7d4c74a1501

Comment 72

[Philip Chee]

Does anything need to be fixed in comm-central? There appears to be only one usage of kungFuDeathGrip in comm-central but my knowledge of C++ is minimal.

https://dxr.mozilla.org/comm-central/rev/ab7a0b257f58/mailnews/base/search/src/nsMsgSearchSession.cpp#538

Comment 73

[Nika Layzell [:nika] (ni? for response)]

(In reply to Philip Chee from comment #72)
> Does anything need to be fixed in comm-central? There appears to be only one
> usage of kungFuDeathGrip in comm-central but my knowledge of C++ is minimal.
> 
> https://dxr.mozilla.org/comm-central/rev/ab7a0b257f58/mailnews/base/search/
> src/nsMsgSearchSession.cpp#538

That use would be permitted by the analysis. If after merging with central you don't get any static analysis failures, you're fine. The static analysis will point out any changes you need to make.

Comment 74

[(no longer active)]

(In reply to Michael Layzell [:mystor] from comment #73)
> (In reply to Philip Chee from comment #72)
> > Does anything need to be fixed in comm-central? There appears to be only one
> > usage of kungFuDeathGrip in comm-central but my knowledge of C++ is minimal.
> > 
> > https://dxr.mozilla.org/comm-central/rev/ab7a0b257f58/mailnews/base/search/
> > src/nsMsgSearchSession.cpp#538
> 
> That use would be permitted by the analysis. If after merging with central
> you don't get any static analysis failures, you're fine. The static analysis
> will point out any changes you need to make.

Note that in order to use static analysis, you need to download a clang binary from llvm.org, use it in your mozconfig, and also add |ac_add_options --enable-clang-plugin| to your mozconfig.

Comment 75

[Philip Chee]

NI rkent && jcranmer see previous two comments.

Comment 76

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Dan, Mike, this bug is fixed in Nightly and Aurora50 is affected. Looking at the amount of code change I am unsure what the risk is of uplifting this to 50. Thoughts?

Comment 77

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ritu Kothari (:ritu) from comment #76)
> Hi Dan, Mike, this bug is fixed in Nightly and Aurora50 is affected. Looking
> at the amount of code change I am unsure what the risk is of uplifting this
> to 50. Thoughts?

I think this is probably a fix which should ride the trains, because uplifting it will be a bit tedious, and it doesn't fix a specific security problem, rather it proactively prevents future security problems.

Ehsan, do you think it's important to try to uplift this analysis?

Comment 78

[Daniel Veditz [:dveditz]]

I lean slightly toward uplifting because I'm positive there are more latent security bugs in there like bug 1005584 and the code changes look straightforward in terms of risk despite the number of them. But I acknowledge merging/back-porting could be a PITA. 50-51 is a loooong cycle though: if we get this into 50 it'll ship in November while 51 won't ship until late January. Is a 7-week beta cycle enough to work out any kinks from this? Have we seen any signs of regression on Nightly in the last week?

Comment 79

[Kent James (:rkent)]

I created Bug 1302845 to respond to the suggestion in comment 74.

Comment 80

[(no longer active)]

I tend to agree with dveditz that backporting the code changes makes sense, since it's close to unimaginable how the code fixes could cause regressions.  Therefore I think at least the code changes need backporting.  And since it's very difficult to ensure that we are catching every single instance on branches, we should probably also backport the analysis.

My fear is that somehow someone would find out what this analysis is intended to do, then run it on aurora/beta and fuzz some stuff and find a vulnerability.  This _does_ seem like a stretch, but I would rather be safe than sorry.

Can you please backport these?

Comment 81

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 10 BETA: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

I was able to rebase everything except for the analysis on top of MOZILLA_AURORA_50_BASE. This is the modified analysis, and another patch with the remaining changes required for the analysis to land.

Comment 82

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 11 BETA: Changes required for beta uplift

These are the remaining changes required for the beta uplift

Comment 83

[Nika Layzell [:nika] (ni? for response)]

Correction FIREFOX_AURORA_50_BASE is what I rebased these on to. I don't actually have a local clone of the beta tree, so it was the easiest way for me to get something similar to what beta looks like now.

Comment 84

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1 BETA: Changes in dom/

Comment 85

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2 BETA: Changes in editor/

Comment 86

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3 BETA: Changes in widget/cocoa/

Comment 87

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 4 BETA: Changes in layout/

Comment 88

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 5 BETA: Changes in docshell/ and uriloader/

Comment 89

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 6 BETA: Changes in parser/, r=peterv

Comment 90

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 7 BETA: Changes in widget/gtk/

Comment 91

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 8 BETA: Various other changes

Comment 92

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 9 BETA: Changes to account for modifications to clang plugin

Comment 93

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8792969 [details] [diff] [review]
Part 11 BETA: Changes required for beta uplift

Approval Request Comment
[Feature/regressing bug #]:N/A
[User impact if declined]:Potential security vulnerabilities in Firefox 50
[Describe test coverage new/current, TreeHerder]:N/A
[Risks and why]: Low risk, very similar patches have been on central for a while , Changes code in a way unlikely to break.
[String/UUID change made/needed]:None

NOTE: this request is for all of the `Part N BETA` marked patches, which _should_ apply cleanly to beta (let me know if I messed something up with that).

Comment 94

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792969 [details] [diff] [review]
Part 11 BETA: Changes required for beta uplift

Sec-high, Dan and Ehsan both recommended uplifting to 50, let's do it.

Comment 95

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1f49c2d12fda
https://hg.mozilla.org/releases/mozilla-beta/rev/7df9c97ef34b
https://hg.mozilla.org/releases/mozilla-beta/rev/bb8325fe4170
https://hg.mozilla.org/releases/mozilla-beta/rev/a38cdca96627
https://hg.mozilla.org/releases/mozilla-beta/rev/0e7cb9862be7
https://hg.mozilla.org/releases/mozilla-beta/rev/f6e34311ee43
https://hg.mozilla.org/releases/mozilla-beta/rev/14c91e05c399
https://hg.mozilla.org/releases/mozilla-beta/rev/11548f0f2aba
https://hg.mozilla.org/releases/mozilla-beta/rev/7912cc5ad078
https://hg.mozilla.org/releases/mozilla-beta/rev/3ec85c548992
https://hg.mozilla.org/releases/mozilla-beta/rev/bbc17cba352e

Do we still need to do something for ESR45 here?

Comment 96

[Ryan VanderMeulen [:RyanVM]]

Bustage follow-up for a hunk from Part 1 that got lost during rebasing.
https://hg.mozilla.org/releases/mozilla-beta/rev/49788f46f9b6

Comment 97

[Nika Layzell [:nika] (ni? for response)]

Talked with RyanVM about this on IRC last night and came up with above solution.

Comment 98

[Nika Layzell [:nika] (ni? for response)]

Do we also want to uplift this to ESR 45, or are we happy with it being in 50+

Comment 99

[(no longer active)]

We should handle ESR similar to other supported branches.

Comment 100

[Daniel Veditz [:dveditz]]

Sadly for you, Ehsan is right -- we need these fixes on ESR. The sooner the better I think given the scope of the changes.

Comment 101

[(no longer active)]

If you need information from me, please specify how I can help!

Comment 102

[Nika Layzell [:nika] (ni? for response)]

Rebasing this onto ESR is going to be a bit of work, and I'm really bogged down with projects right now. I'll try to get it rebased soon.

Comment 103

[Al Billings [:abillings - ex-MoCo]]

Ehsan, my misread. I thought Dan was asking you to do the work.

Comment 104

[(no longer active)]

No worries!

And thanks, Michael.

Comment 105

[Sylvestre Ledru [:Sylvestre]]

Michael, if we want to fix that for the next ESR, we need a patch this week. Thanks.

Comment 106

[Nika Layzell [:nika] (ni? for response)]

Attachment: ESR45: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

Unfortunately, none of the patches from the non-esr bug applied cleanly, so I had to redo the patch on esr45. I don't know if I've made any mistakes which break anything horribly :-S. Hopefully not.

I'm re-attaching a new version of the analysis which should also cleanly apply on esr45.

Comment 107

[Nika Layzell [:nika] (ni? for response)]

Attachment: ESR45: Changes to satisfy KungFuDeathGrip analysis

Comment 108

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8803062 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

I just noticed that this patch seems to have a ton of random files which got added. I'll remove those and request review again.

Comment 109

[Nika Layzell [:nika] (ni? for response)]

Attachment: ESR45: Changes to satisfy KungFuDeathGrip analysis

Hopefully this one doesn't have any random junk which was left over from my attempted cherry-pick.

Comment 110

[(no longer active)]

Comment on attachment 8803062 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

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

r=me if the files added in editor/libeditor were all accidents and you can remove them from the patch.  If there's something else going on, please ask for another review.  Thank you!

::: editor/libeditor/3
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Looks like this was added unintentionally.

::: editor/libeditor/EditorEventListener.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

All of these added files seem wrong.  These were renamed from nsFoo.cpp to Foo.cpp.

Comment 111

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8803144 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec bug.
User impact if declined: This bug was a blanket fix which may have solved security issues. 
Fix Landed on Version: This fix is in Firefox 50+
Risk to taking this patch (and alternatives if risky): There may be some broken-ness in this patch which isn't present in the other patches, as the patch had to be reworked to apply on ESR45.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 112

[Liz Henry (:lizzard) (relman/hg->git project)]

I hope we would catch any possible broken-ness in treeherder tests. 
Is there any other way you can think of to test this on the ESR build?

Comment 113

[Liz Henry (:lizzard) (relman/hg->git project)]

A second review might be a good idea here.

Comment 114

[Liz Henry (:lizzard) (relman/hg->git project)]

Georg, Kai, Martin, glandium, I thought you should have a heads up this is about to land for 45.5.0esr.  
If you have feedback please let us know.

Comment 115

[Liz Henry (:lizzard) (relman/hg->git project)]

bz or smaug, i know you are both busy, but this seems worth possibly bothering you.  
I would love a second reviewer for these patches for some reassurance that it's as good as it can be.

Comment 116

[Olli Pettay [:smaug][[e-mail]]]

Which patches exactly?
mystor, is it possible to get an interdiff comparing the esr45 patches to what landed elsewhere?

Comment 117

[Liz Henry (:lizzard) (relman/hg->git project)]

The two marked for ESR approval, 
https://bugzilla.mozilla.org/attachment.cgi?id=8803061&action=edit
https://bugzilla.mozilla.org/attachment.cgi?id=8803061&action=edit

Comment 118

[Liz Henry (:lizzard) (relman/hg->git project)]

Ugh, I meant the 2nd link to be https://bugzilla.mozilla.org/attachment.cgi?id=8803144&action=edit

Comment 119

[Georg Koppen]

Thanks for the heads-up. Regarding feedback: I tested the two patches and they compiled fine in all our (cross-)compile environments. I gave the resulting Linux bundle a whirl and nothing blew up. So, looks good so far from our POV.

Comment 120

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8803061 [details] [diff] [review]
ESR45: Add an analysis to reject the kungFuDeathGrip pattern on function results and member variables

Adds new tests, let's land this on esr-45.

Comment 121

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8803144 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

Let's land this now so that we will have extra time for testing.
 
Note, the release date for Firefox 50 and for 45.5.0esr has changed - we are pushing it back from Nov. 8th to Nov. 15th. So if we see regressions from this change, we have a little time to push fixes and rebuild.

Comment 122

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8803144 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

>@@ -1239,16 +1240,17 @@ WebSocket::Constructor(const GlobalObject& aGlobal,
>+  Unused << kungfuDeathGrip;

Why is this enough to satisfy the analysis?  In the !NS_IsMainThread() branch we make a call on webSocket->mImpl, then also pass it as an argument to something, as far as I can tell.

On trunk, this function was changed to use webSocketImpl for some of the webSocket->mImpl uses (the updated name of the deathgrip)...  But even there, we seem to poke at mImpl a few times in a row, which is not obviously (to me) safe.  

>@@ -949,16 +950,19 @@ FetchBody<Derived>::ContinueConsumeBody(nsresult aStatus, uint32_t aResultLength
>+  // This acts as a kungfuDeathGrip on this, which is safe, so we can ignore it

This comment is moderately confusing.  Maybe:

  // The kungFuDeathGrip keeps alive this object itself.  The `this` pointer is
  // not really mutable, so we don't have to worry about someone getting a stale
  // value of it, hence we don't need to change any of the code the
  // kungFuDeathGrip protects to using the pointer stored in the
  // kungFuDeathGrip.

or something.

>@@ -60,17 +60,17 @@ MediaEngineTabVideoSource::StopRunnable::Run()

So why do we need to hold a strong ref to mVideoSource->mWindow, but not to mVideoSource?  Seems odd; maybe an analysis false positive, effectively?

>@@ -2570,17 +2570,17 @@ WorkerPrivateParent<Derived>::Freeze(JSContext* aCx, nsPIDOMWindow* aWindow)

This code doesn't make sense to me either.  Is it that Freeze() can only kill the thing at index i but not the next thing in the list?  _And_ it can't remove things from the list?  This all looks very fishy.  Might want a followup bug here to figure out what's _really_ going on.  :(

Similar for Thaw() in this file.

> nsEditorEventListener::HandleEvent(nsIDOMEvent* aEvent)

No, this isn't quite right.  This method calls other methods that use mEditor, even though it doesn't use it itself.

Presumably this means we need to push the deathgrip into the callees that actually use mEditor, or have multiple deathgrips, or pass around the deathgrip value, or this deathgrip is not needed, or something.

Followup bug?

>@@ -115,17 +115,17 @@ nsHTMLEditor::LoadHTML(const nsAString & aInputString)

This is a bit questionable too.  We hold the deathgrip across calls to things like InsertNode, which is good, but do those use mRules?  And if they do, do they depend on it staying alive?  That may not happen if it gets reassigned.

Similar for the other stuff in this datatransfer file.

>@@ -205,17 +206,19 @@ nsHtml5Parser::Parse(const nsAString& aSourceBuffer,

Why is mExecutor's deathgrip not actually being used, with mExecutor used instead?  At least some places; other places the deathgrip _is_ used.

>@@ -349,16 +350,17 @@ nsHtml5TreeOpExecutor::RunFlushLoop()

Similar here: why is mParser still getting used instead of the deathgrip?

>+++ b/toolkit/components/places/nsNavHistoryResult.cpp

Why the change to the QI impl here?  That seems ... odd.

>@@ -1360,16 +1361,17 @@ IMContextWrapper::DispatchCompositionChangeEvent(

This code is fishy too; it takes the deathgrip, then once it goes out of scope pokes at the member?  It doesn't make much sense to me.

Comment 123

[Boris Zbarsky [:bzbarsky]]

Note that some of those questions/issues likely apply to trunk too.

Comment 124

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #122)
> Comment on attachment 8803144 [details] [diff] [review]
> ESR45: Changes to satisfy KungFuDeathGrip analysis
> 
> >@@ -1239,16 +1240,17 @@ WebSocket::Constructor(const GlobalObject& aGlobal,
> >+  Unused << kungfuDeathGrip;
> 
> Why is this enough to satisfy the analysis?  In the !NS_IsMainThread()
> branch we make a call on webSocket->mImpl, then also pass it as an argument
> to something, as far as I can tell.

The analysis isn't super smart.
I've fixed a bunch of the code which you're referring to.

> >@@ -949,16 +950,19 @@ FetchBody<Derived>::ContinueConsumeBody(nsresult aStatus, uint32_t aResultLength
> >+  // This acts as a kungfuDeathGrip on this, which is safe, so we can ignore it
> 
> This comment is moderately confusing.  Maybe:
> 
>   // The kungFuDeathGrip keeps alive this object itself.  The `this` pointer
> is
>   // not really mutable, so we don't have to worry about someone getting a
> stale
>   // value of it, hence we don't need to change any of the code the
>   // kungFuDeathGrip protects to using the pointer stored in the
>   // kungFuDeathGrip.
> 
> or something.

Changed

> >@@ -60,17 +60,17 @@ MediaEngineTabVideoSource::StopRunnable::Run()
> 
> So why do we need to hold a strong ref to mVideoSource->mWindow, but not to
> mVideoSource?  Seems odd; maybe an analysis false positive, effectively?

I mean, that's what the code was doing before, and I didn't want to change it too much. I can also grip mVideoSource if you want.

> >@@ -2570,17 +2570,17 @@ WorkerPrivateParent<Derived>::Freeze(JSContext* aCx, nsPIDOMWindow* aWindow)
> 
> This code doesn't make sense to me either.  Is it that Freeze() can only
> kill the thing at index i but not the next thing in the list?  _And_ it
> can't remove things from the list?  This all looks very fishy.  Might want a
> followup bug here to figure out what's _really_ going on.  :(
> 
> Similar for Thaw() in this file.

This just grips it for the duration of the call to Thaw() / Freeze()? I don't really see what you're talking about.

> > nsEditorEventListener::HandleEvent(nsIDOMEvent* aEvent)
> 
> No, this isn't quite right.  This method calls other methods that use
> mEditor, even though it doesn't use it itself.
> 
> Presumably this means we need to push the deathgrip into the callees that
> actually use mEditor, or have multiple deathgrips, or pass around the
> deathgrip value, or this deathgrip is not needed, or something.
> 
> Followup bug?

I don't feel super comfortable reasoning about editor. I can make a follow up called something like "Investigate death grips in editor for safety concerns"

> >@@ -205,17 +206,19 @@ nsHtml5Parser::Parse(const nsAString& aSourceBuffer,
> 
> Why is mExecutor's deathgrip not actually being used, with mExecutor used
> instead?  At least some places; other places the deathgrip _is_ used.

My bad, fixed.

> >@@ -349,16 +350,17 @@ nsHtml5TreeOpExecutor::RunFlushLoop()
> 
> Similar here: why is mParser still getting used instead of the deathgrip?

The loop repeats until mParser is null, and I didn't want to change the behavior.

> >+++ b/toolkit/components/places/nsNavHistoryResult.cpp
> 
> Why the change to the QI impl here?  That seems ... odd.

It was necessary because this type implements that interface. There was a place where one of these values was being passed around as an interface pointer and then assigned into a nsCOMPtr<interface>. The person trying to do the death grip was writing a do_QueryInterface(pointer) even though the types matched, presumably because it crashed due to nsCOMPtr<T>'s asserts if they didn't do that. Implementing the interface fixes that problem 

> >@@ -1360,16 +1361,17 @@ IMContextWrapper::DispatchCompositionChangeEvent(
> 
> This code is fishy too; it takes the deathgrip, then once it goes out of
> scope pokes at the member?  It doesn't make much sense to me.

I moved it around to use one overall death grip, and use it in as many places as possible. I was tripped up a bit by the `lastFocusedWindow != mLastFocusedWindow` check at the end.

Comment 125

[Nika Layzell [:nika] (ni? for response)]

Attachment: ESR45 Interdiff

This a diff of my changes since the last patch. I will also submit an overall final patch.

Comment 126

[Nika Layzell [:nika] (ni? for response)]

Attachment: ESR45: Changes to satisfy KungFuDeathGrip analysis

MozReview-Commit-ID: AWhXdf20T7d

Comment 127

[Boris Zbarsky [:bzbarsky]]

> I mean, that's what the code was doing before, and I didn't want to change it too much.

OK.  File a followup on the people who own this code?

> This just grips it for the duration of the call to Thaw() / Freeze()? 

Hmm.  Ok.  I'm not sure why this wasn't making sense to me before.

> I don't feel super comfortable reasoning about editor

That's normal.  As I said, followup is great.

> The loop repeats until mParser is null, and I didn't want to change the behavior.

I meant the "GetParser()->GetStreamParser()" bit.  I guess it's preceded by a !mParser null-check, so presumably it's ok.

> There was a place where one of these values was being passed around as an interface pointer
> and then assigned into a nsCOMPtr<interface>. 

Ah, and the upshot was that due to the QI call the deathgrip was actually not working?  That deathgrip isn't part of your patch, right?  How did you find it?

Comment 128

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8805991 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

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

::: storage/mozStorageAsyncStatementExecution.cpp
@@ -70,5 @@
>        // Hold a strong reference to the callback while notifying it, so that if
>        // it spins the event loop, the callback won't be released and freed out
>        // from under us.
> -      nsCOMPtr<mozIStorageStatementCallback> callback =
> -        do_QueryInterface(mCallback);

This line

@@ -106,5 @@
>        // Hold a strong reference to the callback while notifying it, so that if
>        // it spins the event loop, the callback won't be released and freed out
>        // from under us.
> -      nsCOMPtr<mozIStorageStatementCallback> callback =
> -        do_QueryInterface(mCallback);

And this line are the ones which needed the QI implementation changes.

Comment 129

[Nika Layzell [:nika] (ni? for response)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #127)
> > I mean, that's what the code was doing before, and I didn't want to change it too much.
> 
> OK.  File a followup on the people who own this code?

Central doesn't contain that death grip anymore, so I'm not sure what the purpose of it was.

> > I don't feel super comfortable reasoning about editor
> 
> That's normal.  As I said, followup is great.

I already filed bug 1314053

Comment 130

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8805991 [details] [diff] [review]
ESR45: Changes to satisfy KungFuDeathGrip analysis

Updated patch in response to bz's comments

Comment 131

[Liz Henry (:lizzard) (relman/hg->git project)]

Wes can you re-land the new patch in comment 130 on esr45? Thanks.

Comment 132

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/32ce7be985433fd0b3da7ed275ecd5cf29c91587