Re: Proposal to ban the usage of refcounted objects inside C++ lambdas in Gecko
On Fri, Apr 10, 2015 at 10:37 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
wrote:
On 2015-04-10 12:33 PM, Andrew McCreight wrote:
On Fri, Apr 10, 2015 at 8:46 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
mailto:ehsan.akhg...@gmail.com wrote:
I would like to propose that we should ban the usage of refcounted
objects
inside lambdas in Gecko. Here is the reason:
Consider the following code:
nsINode* myNode;
TakeLambda([]() {
myNode-Foo();
});
How is this any worse than any other usage of raw pointers on the stack?
nsINode* myNode;
doSomething();
myNode-Foo();
is the same thing.
It is worse since the lambda may be held on to and executed later in the
future (i.e., past the point in time when the function returns.)
But also do note that raw pointers to refcounted objects on the stack are
very dangerous as well (for example in your above code, doSomething() could
destroy myNode.
I also don't see how don't use any refcounted objects inside lambdas
is any easier to enforce.
It's easier because we don't yet have piles of code which have this
pattern in them. I would like to prevent us getting there so that we don't
have to start solving this issue on a massive scale in a few years once we
grow more lambda usage.
I also don't see why refcounted objects in particular are worth of
exclusion based on their lifetime in closures, but not references to
other allocated things. I guess we don't have local variables like
|nsTArrayFoo* myArray;| as often, but that would be just as dangerous.
Yes, I have nothing against coming up with further restrictions to make
more things safe, but mentioning other dangerous things is not an argument
against this proposal. ;-)
Well, I don't really see what is difficult about this in particular, but
I've been writing code in functional programming languages for a long time,
so maybe I'm just used to the lifetime issues around closures already.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to ban the usage of refcounted objects inside C++ lambdas in Gecko
On 04/10/2015 09:09 PM, Seth Fowler wrote:
On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari ehsan.akhg...@gmail.com wrote:
I would like to propose that we should ban the usage of refcounted objects
inside lambdas in Gecko. Here is the reason:
Consider the following code:
nsINode* myNode;
TakeLambda([]() {
myNode-Foo();
});
There is nothing that guarantees that the lambda passed to TakeLambda is
executed before myNode is destroyed somehow.
I agree that this pattern is bad, but I don’t think that means that we should
ban lambda capture of refcounted objects.
This alternative formulation would work just fine today, AFAIK:
nsCOMPtrnsINode myNode;
TakeLambda([=]() {
myNode-Foo();
});
This captures by value, so we end up with a copy of myNode in the lambda, with
the refcount incremented appropriately.
Once we have C++14 support everywhere, we can also do this:
nsCOMPtrnsINode myNode;
TakeLambda([myNode = Move(myNode)]() {
myNode-Foo();
});
To capture by move (and avoid the cost of a refcount increment).
Using either of these approaches is enough to make refcounted objects safe to
use in lambda capture expressions. Lambdas will be much less useful if they
can’t capture refcounted objects, so I’m strongly against banning that.
I'd say that is rather painful for reviewers, since both Move() (I prefer
.swap()) and lambda hide what is actually happening to the refcnt.
So easy to forget to use nsCOMPtr explicitly there.
We should emphasize easy-to-read-and-understand code over fast-to-write.
-Olli
- Seth
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Firefox/Platform Engineering Update 39.3
This engineering update is also available on the Platform wiki and my blog: https://wiki.mozilla.org/Platform/2015-04-14 http://cpeterso.com/blog/02015/04/mozilla-engineering-update-39-3/ # Firefox Release Schedule (lmandel) - We shipped a Firefox 37.0.1 desktop and mobile chemspills last week. - Firefox 38 Beta 1 shipped last week. - Updates have been re-enabled for Aurora. - 5 weeks until the next merge date (2015-05-11). # Upcoming Outages (hwine) Tree Closing Window on Saturday April 18 for at least 4-5 hours of work (time TBD). Tracking bug 1151645. # MemShrink (njn) Brian Hackett took advantage of the removal of object parents to reduce the size of base shapes (bug 1143256). The JS engine creates many of these to track the characteristics of JS objects. Nick Fitzgerald greatly reduced the amount of memory used for saving JS exception stacks (bug 1038238). This reduced the memory of a test case compiled from Dart to JS from over 1GB to around 170MB (bug 1125259). Seth Fowler fixed a bad leak regression (bug 1150127) where closing a page while it was in the process of loading could leak the page and cause long browser pauses. Seth also fixed a pair of issues that caused very large memory usage (more than 1GB) when scrolling in the downloads list (bug 1148682 and bug 1148684). Bill McCloskey fixed an issue where a docshell was being held alive for too long on e10s (bug 1137933). This was one of the remaining blockers to enabling e10s Mochitest browser-chrome tests, which will give us more test coverage for leaks. # Media (cpeterson) We shipped MSE (Media Source Extensions) for YouTube in Firefox 37 on Windows Vista+. Unfortunately, we discovered multiple unrelated issues in the release channel that all manifest themselves as “video is black”: - bug 1151721: Intel GPU driver bug (will be hotfixed by bug 1152630) - bug 1151638: NoScript blocking MSE JS (will be fixed soon in NoScript 2.6.9.21) - bug 1135078: IPv6 problems (partially fixed bug 1135078) YouTube helped us quickly revert all Firefox users from HTML5 video to Flash. Once the problems were identified, YouTube was able to switch OS X and Windows Vista+ pre-channels back to HTML5 video. We plan to ship MSE for YouTube on Windows Vista+ in Firefox 38 (or 37 if hotfix bug 1152630 is released soon) and on OS X in Firefox 38 (or possibly slip to 39). ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Intent to implement and ship: Settable .files attribute on HTMLInputElement
On Fri, Apr 10, 2015 at 11:24 AM, Boris Zbarsky bzbar...@mit.edu wrote: 1) The setter treats its single argument as a sequenceBlob. Any Blobs that are not already Files get upgraded to Files, identical to how https://xhr.spec.whatwg.org/#create-an-entry behaves. Is there a reason to upgrade to File? I thought it'd be good to keep the object identities the same as much as possible (except for the sequence object itself of course). / Jonas ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Intent to implement and ship: Settable .files attribute on HTMLInputElement
On 4/10/15 6:23 PM, Jonas Sicking wrote: Is there a reason to upgrade to File? Yes. FileList claims to return Files. We could change that to have it return Blobs, of course, and then make sure that various consumers (at least the internal ones) can deal. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Intent to implement and ship: Settable .files attribute on HTMLInputElement
On Fri, Apr 10, 2015 at 4:31 PM, Boris Zbarsky bzbar...@mit.edu wrote: On 4/10/15 6:23 PM, Jonas Sicking wrote: Is there a reason to upgrade to File? Yes. FileList claims to return Files. We could change that to have it return Blobs, of course, and then make sure that various consumers (at least the internal ones) can deal. I think that's the way to go. One option is to make the xpidl interface always return a File, but make the .webidl interface return files or blobs. We'd still have to check internal JS consumers though. But I think that's the right thing to do. / Jonas ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Proposal to ban the usage of refcounted objects inside C++ lambdas in Gecko
On Fri, Apr 10, 2015 at 11:46 AM, Ehsan Akhgari ehsan.akhg...@gmail.com
wrote:
I would like to propose that we should ban the usage of refcounted objects
inside lambdas in Gecko. Here is the reason:
Consider the following code:
nsINode* myNode;
TakeLambda([]() {
myNode-Foo();
});
There is nothing that guarantees that the lambda passed to TakeLambda is
executed before myNode is destroyed somehow. While it's possible to verify
on a case by case basis that this code is indeed safe, I think it's too
error prone to keep these invariants over time. Given the fact that if
things go wrong we'd deal with a UAF bug which will be sec-critical, I
think it's better to be safe than sorry here.
If nobody objects, I'll update the style guide in the next few days. If
you do object, please make a case for why the above analysis is incorrect
and why the usage of refcounted objects in lambdas in general is safe.
Why do we merely update the style guide? We have a custom Clang compiler
plugin. I think we should have the Clang compiler plugin enforce our style
guidelines by refusing to compile code that is against policy. This would
enable reviewers to stop focusing on policing the style guide and enable
them to focus more on the correctness of code. This will result in faster
reviews and/or higher quality code.
We already do some of this with the existing Clang compiler plugin. But
it's far from comprehensive. I think we should strive for a world where
machines, not humans, are enforcing the style guide.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
Re: Intent to implement and ship: Settable .files attribute on HTMLInputElement
On 4/10/15 7:43 PM, Jonas Sicking wrote: One option is to make the xpidl interface always return a File, but make the .webidl interface return files or blobs. Why, exactly? If we want to do this, just have them both return a Blob in terms of the IDL. If it happens to be a Blob that's also a File, fine. We'd still have to check internal JS consumers though. But I think that's the right thing to do. I'm more worried about internal C++ consumers, honestly. Have to audit them all to make sure they're not assuming they can get a filename and such. It'd be less painful to audit this if we didn't internally use File to refer to Blobs. :( -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: Intent to implement and ship: Settable .files attribute on HTMLInputElement
On 4/10/15 10:15 PM, Boris Zbarsky wrote: I'm more worried about internal C++ consumers, honestly. Have to audit them all to make sure they're not assuming they can get a filename and such. For example, HTMLInputElement::GetValueInternal or HTMLInputElement::AfterSetFiles will do mFiles[0]-GetMozFullPath. This will land in FileImplBase::GetMozFullPath which will proceed to assert mIsFile. Which will be false if mFiles[0] is a Blob... I'm sure there's other fun bits like that hiding in there too. -Boris ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
