/builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsDBFolderInfo.h:34:3: error: bad implicit conversion constructor for 'nsDBFolderInfo' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsDBFolderInfo.h:34:3: error: bad implicit conversion constructor for 'nsDBFolderInfo' /builds/worker/workspace/build/src/comm/mailnews/db/msgdb/src/nsDBFolderInfo.cpp:135:17: error: bad implicit conversion constructor for 'nsDBFolderInfo' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.h:41:5: error: bad implicit conversion constructor for 'TokenEnumeration' /builds/worker/workspace/build/src/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.h:97:5: error: bad implicit conversion constructor for 'TokenHash' /builds/worker/workspace/build/src/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:121:19: error: bad implicit conversion constructor for 'TokenEnumeration' /builds/worker/workspace/build/src/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:147:12: error: bad implicit conversion constructor for 'TokenHash' /builds/worker/workspace/build/src/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:938:5: error: bad implicit conversion constructor for 'TokenStreamListener' /builds/worker/workspace/build/src/comm/mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp:951:22: error: bad implicit conversion constructor for 'TokenStreamListener' /builds/worker/workspace/build/src/comm/mailnews/imap/src/nsAutoSyncState.h:57:3: error: bad implicit conversion constructor for 'nsAutoSyncState' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsMsgSearchBoolExpression.h:44:3: error: bad implicit conversion constructor for 'nsMsgSearchBoolExpression' /builds/worker/workspace/build/src/comm/mailnews/base/search/src/nsMsgSearchValue.h:14:5: error: bad implicit conversion constructor for 'nsMsgSearchValueImpl' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsMsgSearchBoolExpression.h:44:3: error: bad implicit conversion constructor for 'nsMsgSearchBoolExpression' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/comm/mailnews/compose/src/nsURLFetcher.h:88:3: error: bad implicit conversion constructor for 'nsURLFetcherStreamConsumer' /builds/worker/workspace/build/src/comm/mailnews/base/src/nsMailDirProvider.h:38:5: error: bad implicit conversion constructor for 'AppendingEnumerator' /builds/worker/workspace/build/src/comm/mailnews/base/src/nsMailDirProvider.cpp:193:41: error: bad implicit conversion constructor for 'AppendingEnumerator' /builds/worker/workspace/build/src/comm/mailnews/base/util/nsMsgKeySet.h:83:3: error: bad implicit conversion constructor for 'nsMsgKeySet' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsDBFolderInfo.h:34:3: error: bad implicit conversion constructor for 'nsDBFolderInfo' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsMsgKeySet.h:83:3: error: bad implicit conversion constructor for 'nsMsgKeySet' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsMsgKeySet.h:83:3: error: bad implicit conversion constructor for 'nsMsgKeySet' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' /builds/worker/workspace/build/src/comm/mailnews/compose/src/nsMsgSendPart.h:25:5: error: bad implicit conversion constructor for 'nsMsgSendPart' /builds/worker/workspace/build/src/obj-thunderbird/dist/include/mozilla/mailnews/MimeHeaderParser.h:50:3: error: bad implicit conversion constructor for 'UTF16ArrayAdapter' Linux people step forward, this compiles for me on Windows, even with clang-cl. Maybe caused by bug 1486654.

Just pulled latest from m-c and c-c and it seems to compile OK for me (Debian Linux, looks mozbuild seems to be using clang 6.0.1). I assume the log is from a build server somewhere? I'll poke around the log errors a little more and see if I can spot anything.

It's a bit of a mystery to me. But see: https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=814db0d1e0667dd07bf9fc760444e77bb54a24eb&selectedJob=196485667 with the log here: https://taskcluster-artifacts.net/dWdSO79HSJCONn_cyr2bHA/0/public/logs/live_backing.log Yes, they switched to clang a while (1-2 weeks) ago.

Yes, this built for me too. Ubuntu 16.04, also with Clang 6.0.1. Weird.

Mike Hommey confirms it's enabling static analysis that caused the errors to show up. We should probably fix the errors, or we can turn static analysis off again. Ben, is this a good one for you?

Looking into the static analysis stuff now. I'll try and get the errors occurring locally and see if I can figure out any fixes. Passing cleanly through static analysis definitely seems like a good goal :-)

For reference, it looks like this static analysis rule was introduced via Bug 1009631. c-c seems to get away reasonably lightly compared to m-c.

Here's a patch I _think_ should do the trick... it compiles fine for me, but I've not yet run it through static analysis. (takes too long locally, and I'm having some trouble pushing up to the try server - will keep at it and report any progress back here).

I've pushed it to try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=30d014d4fd3574ba64b908aa04c09a6c8c8d6a29

Nope, didn't build on try

Yes, but some of the compile errors we see on C-C have now disappeared, like "mailnews/MimeHeaderParser.h:50". So this appears to be going in the right direction. What's the improvement of adding 'explicit' in a few places in terms of code quality?

(In reply to Jorg K (GMT+2) from comment #10) > What's the improvement of adding 'explicit' in a few places in > terms of code quality? Example: class FancyThing { public: FancyThing(int n); }; This would be fine, without explicit:: FancyThing a = 42; I can see there'd be places where it'd really simplify syntax, but I can also see it could lead to real confusion, eg: FancyArray arr = 16; // length 16? capacity 16? containing single element 16? So with the conversion ctor marked explicit, you have to be... uh... explicit about it, ie: FancyThing b = FancyThing(42); m-c had to patch a lot of stuff into this second form. Ours is simple enough so far. Gets even more complicated in C++11, as you can do stuff like: OtherFancyThing foo = {"hello", 42}; or return {"hello",42}; to invoke multiple-param conversion ctors...

Geoff provided this patch which I'll land to get Linux going again.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/909cf6e1db3f disable static analysis on Linux. rs=bustage-fix,jorgk

I've been through all the additional "bad implicit conversion constructor" I could find in Geoff's try-server run above. I'm just getting my try-server access sorted out, and then I'll run it myself. If I just reverse Geoff's patch to various "mail/config/mozconfigs/..." files in my local repo and push that up to the try server, that should run all the static analysis stuff for me again, right?

Yes, I'd do "hg qbackout -r 909cf6e1db3f", that will give you a patch.

Oh, Geoff's try server run popped up another few errors too: comm/mailnews/compose/src/nsSmtpProtocol.cpp:2312:40: error: calling `get` on a temporary, potentially allowing use after free of the raw pointer comm/mailnews/base/util/nsImapMoveCoalescer.cpp:116:5: error: Unused "kungFuDeathGrip" 'nsCOMPtr<nsIUrlListener>' objects constructed from temporary values are prohibited (there are another 3 kungFuDeathGrip warnings) I understand what's happening on these, just working out what the best approach is to fix 'em. But I'll probably keep such fixes in a separate patch for easier review, as it's xpcom territory, where I'm still finding my way.

This patch fixes up the other few static-analysis/lint errors I saw. Not much to it, but could I get someone to give it an eyeball to make sure I've not screwed it up? I can merge them back into a single patch if preferred.

Comment on attachment 9006150 [details] [diff] [review] explicit_conversion_ctors_2.patch Thanks. This one is a little monotonous, but if that's what we need to do ;-) - BTW, best to keep the patches separate.

Comment on attachment 9006152 [details] [diff] [review] fix_unused_kungfudeathgrip_1.patch Review of attachment 9006152 [details] [diff] [review]: ----------------------------------------------------------------- OK, you found a really bad one and some unused code. Can you please explain the do_QueryInterface() replacements. Oh, I see it now. You get an error if the result isn't used, but only the 'rv'. I'll give it a try run once I've done some other stuff. ::: mailnews/compose/src/nsSmtpProtocol.cpp @@ -2310,3 @@ > const char16_t *formatStrings[] = > { > - NS_ConvertASCIItoUTF16(hostname).get(), Wow, that's a bad one. I made this mistake once and the error message never came out correctly.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=9f807186f48f3f7922dec2be625f9fc6890061b5 Reading the commit message on the second patch I was a little surprised: Bug 1487082 - fix unused-kungFuDeathGrips and mozilla-dangling-on-temporary. r=jorgk Tidies up a few static-analysis/lint errors (rather than compile errors) They sure look like compile errors: nsSmtpProtocol.cpp:2312:40: error: calling `get` on a temporary, potentially allowing use after free of the raw pointer The output appears to be created by clang++. Hmm, but for the other error I see: Please switch all accesses to this value to go through 'urlListener', or explicitly pass 'urlListener' to `mozilla::Unused` and surely that's a Mozilla thing. That makes me think whether we should stick with the QI(xxx, &rv); version and just add mozilla::Unused << xxx;

Here's the backout patch for your convenience. The try run showed more errors.

Previous try had other compile bustage, so here's new one: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8ab2644db4de2012cbe2635cb330aa44014f1947

Well, I don't get any compile errors when I do: ./mach build but I do get them when I do (for example): ./mach static-analysis check comm/mailnews/compose/src/nsSmtpProtocol.cpp I'm a little hazy on what's happening, but as I understand it there seems to be a bunch of checks implemented as a clang plugin. Some of them are definitely mozilla-specific (eg Bug 1009631 adds the explicit conversion ctor check), but I suspect a bunch of them are off-the-shelf (eg there seem to be a bunch of modernizing ones to point out places where >=C++11 features might improve things). Oddly, when I run `./mach static-analysis check` on a single file I get a whole bunch of extra errors (against m-c stuff) which we don't see in the try-server output. So I guess that somewhere a bunch of checks are being suppressed for the try-server build. Anyway, seems like a net win overall if it points out the odd genuine bad bug. Tomorrow I'll pick my way through the logs for the try-server run you did (thanks, btw!) and try and mop up the stragglers - there are a few differences between release and debug builds too. Hopefully I'll have my own try-server access sorted out soon too.

The latest batch of fixes (I'm sure there'll be a few more)... Just to sanity check: the first change in this patch removes a redundant nsISupports check in mailnews/addrbook/src/nsAbLDAPDirectory.cpp It _is_ a redundant check, right? I'm not missing something am I? There's not some subtle way that the object might get get deleted during the function because I removed that nsISupports kungfudeathgrip is there?

Comment on attachment 9006417 [details] [diff] [review] fix_unused_kungfudeathgrip_2.patch Review of attachment 9006417 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/addrbook/src/nsAbLDAPDirectory.cpp @@ -307,4 @@ > rv = GetAttributeMap(getter_AddRefs(attrMap)); > NS_ENSURE_SUCCESS(rv, rv); > > - nsCOMPtr<nsISupports> typeSpecificArg = do_QueryInterface(attrMap, &rv); I think this can be removed. If we want to add an additional reference, we use the variable name kungFuDeathGrip: https://searchfox.org/comm-central/search?q=kungfudeathgrip&case=false&regexp=false&path=mailnews%2F

You have try access now, right?

Running now (finally!). Will check up on it in the morning! https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=85bc34a33e52dd6dc9e58817facea3673a191c11

...and another batch of errors to fix and add to the patches. It looks like the try-server output clips the messages to some set number. I'll keep at it, between doing other stuff. Would be easiest if I could just run the same clang-plugin tests locally, then just power through them all in one go. Not sure why the try-server is running them and my local ./mach build is not. Will poke about and see if I can figure it out.

Shouldn't you be using e.g. /mach static-analysis check comm/mailnews/addrbook/src/nsAbLDAPDirectory.cpp

(In reply to Magnus Melin from comment #31) > Shouldn't you be using e.g. > > /mach static-analysis check > comm/mailnews/addrbook/src/nsAbLDAPDirectory.cpp Yes, but it means going through all the source files one-by-one, and it also returns a whole _heap_ of warnings which don't get triggered on the try-server builds (loads of C++ modernisation tips).

Still plugging away at these... I think it probably makes sense to try and land what we've got so far. There are a couple of trickier fixes coming up and I think it'll be easier for someone to sanity-check them without all the kingfudeathgrip noise.

Comment on attachment 9008996 [details] [diff] [review] explicit_conversion_ctors_4.patch Review of attachment 9008996 [details] [diff] [review]: ----------------------------------------------------------------- Hi Ben, you need to nominate a reviewer, with r?xxx so it shows up in someone's review queue. I'm happy to take further reviews here. ::: db/mork/src/morkParser.cpp @@ +119,4 @@ > , mParser_ColumnSpool(ev, &mParser_ColumnCoil) > , mParser_StringSpool(ev, &mParser_StringCoil) > > +, mParser_MidYarn(ev, morkUsage(morkUsage_kMember), ioSlotHeap) Interesting one. So mParser_MidYarn is a morkYarn and its constructor takes a morkUsage as its second argument. Previously we just passed morkUsage_kMember which is defined to me an 'm'. It appears strange to me that that ever compiled. But I guess that's what this implicit stuff is all about.

Comment on attachment 9009002 [details] [diff] [review] fix_unused_kungfudeathgrip_3.patch Have you considered my comment #21, last paragraph? That makes me think whether we should stick with the QI(xxx, &rv); version and just add mozilla::Unused << xxx; What do you think? Once we settled this, I'm happy to land the first two parts.

Comment on attachment 9005596 [details] [diff] [review] 1487082-disable-static-analysis.patch This needs to go to the beta since it's busted otherwise :-(

Why does this remove checking of rv? What is the benefit and why is it correct style?

(In reply to :aceman from comment #38) > Why does this remove checking of rv? What is the benefit and why is it > correct style? Well, the compiler or static analysis complains about QI(xxx, &rv) if xxx is not used later. There are two solutions to that: 1) Lose the rv and do: QI(xxx), if (!xxx) ... 2) Keep the rv and "treat" xxx: QI(xxx, &rv); mozilla::Unused << xxx; if (NS_FAILED(rv) ...

So how does m-c solve this? Also, we could check both. I don't think there is a rule that the returned pointer must be nulled in case of failure. If you pass in non-null, the same non-null is passed out, only rv indicates failure.

But what would in the case of QI(xxx, &rv) 'xxx' not be used later? We check rv and then use xxx. So what is the problem?

(In reply to :aceman from comment #40) > Also, we could check both. I don't think there is a rule that the returned > pointer must be nulled in case of failure. I think the returned pointer is guaranteed to be null on failure for do_QueryInterface(xxx) (which is what the patch is using).

Sorry, I wrote nonsense. It's |nsCOMPtr<Iiii> xxx = do_QueryInterface(yyy, &rv);| The xxx is unused. So either 1) Lose the rv and do: if (!xxx) ... 2) Keep the rv and "treat" xxx: mozilla::Unused << xxx; if (NS_FAILED(rv) ...

Do you have a preference, Jörg? I'm pretty agnostic on which way we go, although if pushed I'd probably lean toward `if(!xxx)` just because it's less noisy. Besides all that I've a gut feeling that most of the places that check for the interface without subsequently using it should probably either: a) actually _use_ the returned interface (instead of the existing pointer to the more generic interface) or b) not even perform the check in the first place :-)

(In reply to Jorg K (GMT+2) from comment #43) > Sorry, I wrote nonsense. It's |nsCOMPtr<Iiii> xxx = do_QueryInterface(yyy, > &rv);| The xxx is unused. So either But why is xxx unused? Why do we then even fetch it?

As a test. Check this example: - nsCOMPtr<nsIAbMDBDirectory> dbdir(do_QueryInterface(dir, &rv)); - NS_ENSURE_SUCCESS(rv, rv); + nsCOMPtr<nsIAbMDBDirectory> dbdir(do_QueryInterface(dir)); + if (!dbdir) { + return NS_ERROR_NO_INTERFACE; + } So the question is whether this is more ugly than this: nsCOMPtr<nsIAbMDBDirectory> dbdir(do_QueryInterface(dir, &rv)); + mozilla::Unused << dbdir; NS_ENSURE_SUCCESS(rv, rv); Maybe the second variant is OK in order to avoid producing an error ourselves.

(In reply to Jorg K (GMT+2) from comment #46) > Maybe the second variant is OK in order to avoid producing an error > ourselves. Ahh yes! Good point - I'm convinced.

I've reduced this patch to just kungfudeathgrip fixes and tidied it up a bit with `mozilla::Unused<<xxx`. I also deleted a couple of the QI calls altogether, where it was a class checking to see that it supported an interface that it specifically derived from.

this one was part of the kingfudeathgrip patch, but I figured it was neater to split it out.

And with that, it _looks_ like we're down to one error left (fingers crossed) https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=68defff73625d82cadd35b28b7ca11df60cece27 looks like some fun C++11 reading-up for me tomorrow... lambdatastic!

Comment on attachment 9009527 [details] [diff] [review] explicit_conversion_ctors_5.patch Not the most exiting reading, plus I've seen it before ;-)

Comment on attachment 9009529 [details] [diff] [review] double_refcount_1.patch Please educate the reviewer ;-) - When do you use one, and when the other?

Comment on attachment 9009530 [details] [diff] [review] fix_conversion_1.patch This is the worst of the lot. How did that ever work? I coded one like this once and it crashed immediately. But the code happily calls PromptForPassword() and that appears to work since I haven't seen any complaints. I'll uplift this. Thanks for separating it out.

Comment on attachment 9009528 [details] [diff] [review] fix_unused_kungfudeathgrip_4.patch Review of attachment 9009528 [details] [diff] [review]: ----------------------------------------------------------------- The amount of dead code is just amazing. Please adjust your mercurial.ini to contain: [diff] git = 1 showfunc = 1 unified = 8 We need more context. Almost ready, but I don't understand two removals. I visited most of the source files to see the context. ::: mailnews/addrbook/src/nsAbMDBDirectory.cpp @@ +557,4 @@ > if (!hasDir) > return NS_ERROR_NULL_POINTER; > > + nsresult rv=NS_OK; Nit: Spaces around the equals sigh: rv = NS_OK; I'll fix it before landing. @@ -559,5 @@ > > - nsresult rv; > - > - nsCOMPtr<nsIAbMDBDirectory> dbdir(do_QueryInterface(dir, &rv)); > - NS_ENSURE_SUCCESS(rv, rv); Why is this OK to remove? @@ -851,5 @@ > rv = list->GetIsMailList(&bIsMailList); > NS_ENSURE_SUCCESS(rv,rv); > > - nsCOMPtr<nsIAbMDBDirectory> dblist(do_QueryInterface(list, &rv)); > - NS_ENSURE_SUCCESS(rv,rv); And here?

(In reply to Jorg K (GMT+2) from comment #54) > Comment on attachment 9009529 [details] [diff] [review] > double_refcount_1.patch > > Please educate the reviewer ;-) - When do you use one, and when the other? `NS_DECL_ISUPPORTS` [1] adds a protected `mRefCnt` member. If you're implementing `nsISupports`. but also inheriting from something that already has `nsISupports` you'll end up with two `mRefCnt` members (albeit in different parent classes). So `NS_DECL_ISUPPORTS_INHERITED` [2] appears to be the work-around in such cases - it declares all the ISupport functions, but not the refcount (which I assume the child class is meant to delegate to the super class). [1] https://dxr.mozilla.org/mozilla-central/rev/5165e750ffabb930deb846410ab62dcc6d1f9e52/xpcom/base/nsISupportsImpl.h#394 [2] https://dxr.mozilla.org/mozilla-central/rev/5165e750ffabb930deb846410ab62dcc6d1f9e52/xpcom/base/nsISupportsImpl.h#1135

(In reply to Jorg K (GMT+2) from comment #53) > Not the most exiting reading, plus I've seen it before ;-) Heh - sorry about that. Intention was just to flag it up as "I'm happy with this one now and I won't make any more changes except via review feedback".

I added the ;-) to mark the joke. Of course some reviews are more exiting that others. No offence intended.

(In reply to Jorg K (GMT+2) from comment #56) > Almost ready, but I don't understand two removals. I visited most of the > source files to see the context. > > ::: mailnews/addrbook/src/nsAbMDBDirectory.cpp > @@ -559,5 @@ > > > > - nsresult rv; > > - > > - nsCOMPtr<nsIAbMDBDirectory> dbdir(do_QueryInterface(dir, &rv)); > > - NS_ENSURE_SUCCESS(rv, rv); > > Why is this OK to remove? > > @@ -851,5 @@ > > rv = list->GetIsMailList(&bIsMailList); > > NS_ENSURE_SUCCESS(rv,rv); > > > > - nsCOMPtr<nsIAbMDBDirectory> dblist(do_QueryInterface(list, &rv)); > > - NS_ENSURE_SUCCESS(rv,rv); > > And here? Doh. I'm pretty sure I just screwed up on those two. I'll fix them up, check the rest of the patch again and repost (with more diff context!).

The rest was fine. As I said, I checked it all out in the original source.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f4212f66b639 make conversion CTORs explicit. r=jorgk https://hg.mozilla.org/comm-central/rev/71ff55c3ec2e fix a couple of double-mRefCnt declaration errors. r=jorgk https://hg.mozilla.org/comm-central/rev/50da05f6af45 Fix unsafe use of NS_ConvertASCIItoUTF16(). r=jorgk

Comment on attachment 9009779 [details] [diff] [review] fix_unused_kungfudeathgrip_5.patch Thanks. kungFuDeathGrip has its use: https://dxr.mozilla.org/comm-central/search?q=kungFuDeathGrip&redirect=false but these aren't valid cases. So what's the warning exactly? I'll land that together with the last part and the backout (unless I'm desperately looking for a patch to land, in which case it might land earlier).

The kungfudeathgrip errors were this kind of thing: builds/worker/workspace/build/src/comm/mailnews/news/src/nsNewsDownloader.cpp:178:5: error: Unused "kungFuDeathGrip" 'nsCOMPtr<nsIMsgNewsFolder>' objects constructed from temporary values are prohibited The backout patch probably should probably be held back - it still won't build on the try server: /builds/worker/workspace/build/src/obj-thunderbird/dist/include/nsIURIMutator.h:449:29: error: Refcounted variable 'aArgs' of type 'nsIURI' cannot be captured by a lambda I'm poking at this one now.

https://hg.mozilla.org/releases/comm-esr60/rev/65487672fbee3d93a274641f5e2e43afd7c56a4a Landed the SMTP patch so I can sleep better.

Comment on attachment 9005596 [details] [diff] [review] 1487082-disable-static-analysis.patch Landed before the merge to TB 63 beta, hence no uplift needed.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/b1b281b4ba9a fix 'Unused "kungFuDeathGrip"' errors. r=jorgk DONTBUILD

https://hg.mozilla.org/releases/comm-beta/rev/fea261d7ea3c434ff8146c1c3134d306906cd068 https://hg.mozilla.org/releases/comm-beta/rev/27d9b710a257f7ce03e9b138146be4bad3897e81 https://hg.mozilla.org/releases/comm-beta/rev/8b6796066a05c5ac01714fa2276ee3429ebb88cc https://hg.mozilla.org/releases/comm-beta/rev/35cda8f6b181c1e16e1a3e3fbaad7c4b1a147b35 Somehow disabling the errors didn't work on comm-beta, the "disable patch" is present, however, I see those errors. So I decided to land the lot on beta. Now we're eagerly awaiting the last part. Check: https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=35cda8f6b181c1e16e1a3e3fbaad7c4b1a147b35 in a moment so see which errors are there, hopefully only the last one.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=22f47a8e4fc01a052233e24106a6449bfdb2f706 wild guess based on https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/chrome/nsChromeProtocolHandler.cpp#85 and the fact that when you remove the 'PromiseFlatCString' altogether you get this in clang-cl: 0:10.27 c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsIURIMutator.h(444,10): error: call to implicitly-deleted copy constructor of '(lambda at c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\nsIURIMutator.h:444:10)' So the lambda is close ;-)

I think this should do it, modelled on: https://searchfox.org/mozilla-central/rev/a0333927deabfe980094a14d0549b589f34cbe49/chrome/nsChromeProtocolHandler.cpp#81 https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b12a407d97a1d80d199e004fe868e7065411f27a

Just some search breadcrumbs for next time I hit the "Refcounted variable xxx of type yyy cannot be captured by a lambda" issue: <bcampbell> I _think_ the intention of the error is that it's unsafe to have a raw xpcom object in the closure without incrementing the refcnt (as something else could kill the object before the closure is executed).

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/f2758a1d5320 fix 'cannot be captured by a lambda' error in nsLDAPURL.cpp. r=bcampbell https://hg.mozilla.org/comm-central/rev/4d57e86cdce5 Backed out changeset 909cf6e1db3f to re-enable static analysis. a=backout

TB 63 beta: https://hg.mozilla.org/releases/comm-beta/rev/14a94bc48d62a686401fd4faaec79a1b97df844c https://hg.mozilla.org/releases/comm-beta/rev/87060ceb0f456138402123bf4f12a160f65d9b21