Re: Using references vs. pointers in C++ code

[Nicholas Nethercote]

On Wed, May 10, 2017 at 10:27 AM, Karl Tomlinson  wrote:

>
> Or is NotNull really too awkward IRL?
>

I wrote NotNull.h, and I've used it in various places. I'm ambivalent about
it. It does make things clear, but it also is a bit annoying to use. The
code tends to end up with WrapNotNull() and get() calls littered
throughout. It also doesn't currently work with UniquePtr.

The two big comments in mfbt/NotNull.h have some discussion about use cases
for references and pointers and NotNull that is relevant to this thread:
https://dxr.mozilla.org/mozilla-central/rev/b21b974d60d3075ae24f6fb1bae75d0f122f28fc/mfbt/NotNull.h#10-99

I also fiddled around with a MaybeNull type, to explicitly mark pointers
that are nullable, but it was more complex and less obviously useful than
NotNull, so I gave up on it.

Nick
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[L. David Baron]

On Wednesday 2017-05-10 02:43 +0200, Emilio Cobos Álvarez wrote:
> The issue I have with per-type conventions is that it doesn't scale very
> well, specially when you're writing new code (and more specially if
> they're not documented in the style guide).
> 
> What should the convention be for `ServoStyleSet` (which is what
> triggered this thread)? Who decides that, and based on which arguments?

In this particular case, I think it's easy.  ServoStyleSet is the
Stylo version of a class (nsStyleSet) that uses pointer conventions
because it used to implement XPCOM interfaces.  We shouldn't treat
the two style set classes differently.  So it should be passed with
pointers, not references.

-David

-- 
턞   L. David Baron http://dbaron.org/   턂
턢   Mozilla  https://www.mozilla.org/   턂
 Before I built a wall I'd ask to know
 What I was walling in or walling out,
 And to whom I was like to give offense.
   - Robert Frost, Mending Wall (1914)


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Emilio Cobos Álvarez]

On Tue, May 09, 2017 at 02:11:41PM -0700, L. David Baron wrote:
> On Tuesday 2017-05-09 11:58 +0200, Emilio Cobos Álvarez wrote:
> > I think references help to encode that a bit more in the type system,
> > and help reasoning about the code without having to look at the
> > implementation of the function you're calling into, or without having to
> > rely on the callers to know that you expect a non-null argument.
> > 
> > Personally, I don't think that the fact that they're not used as much as
> > they could/should is a good argument to prevent their usage, but I don't
> > know what's the general opinion on that.
> 
> I have two general concerns about references:
> 
>  (1) When reading the calling code that calls a function taking a
>  non-const reference, it's not obvious that the function might
>  mutate the value, particularly in code that uses references
>  rarely, or for types that are rarely passed as references.

Isn't it similarly non-obvious for code that takes non-const pointers
when the object isn't in the stack?

For example, if I have `void foo(Element*)`, which calls
`void bar(Element*)`, and both assume a non-null argument, I don't see
how potential mutation is more obvious there than `void foo(Element&)`
calling `void bar(Element&)`.

(Other people are concerned about non-const references used for
outparams. I think I'll be fine with the rule Nathan proposed in the
thread as a rule of thumb).

>  (2) Code that mixes functions that take references and functions
>  that take pointers, for the same time, is a mess, because you
>  constantly have to worry about whether to write * or &.

I agree with this, though I don't think writing & or * is a big deal in
most cases.

>  Thus, in Gecko, we generally use pointers or references per-type.
>  XPCOM interfaces are passed by pointer.  String classes are
>  passed by reference.  etc.
>
> I think per-type conventions for using references versus pointers
> helps with both (1) and (2), and I consider it part of Gecko style
> even if it's not well-documented.

The issue I have with per-type conventions is that it doesn't scale very
well, specially when you're writing new code (and more specially if
they're not documented in the style guide).

What should the convention be for `ServoStyleSet` (which is what
triggered this thread)? Who decides that, and based on which arguments?

> I'm also not sure how much references help with enforcing
> non-null-ness.  People will just write a "*" whether or not their
> existing code guarantees a non-null pointer; at least that was my
> experience in the early days of Gecko.  Perhaps we have better code
> review now... but I'm somewhat skeptical of that given that I think
> we've been relying on tests more and review less?

I'm not talking about enforcing non-null-ness, because as you've pointed
out, references don't necessarily do that.

I think the main benefit is at the time of both reading and reasoning
about the code. Using references allows you to quickly figure out what
kind of thing a function expects, handles, or returns, without having to
look at the function itself and potentially all the callers.

 -- Emilio

> -David
> 
> -- 
> 턞   L. David Baron http://dbaron.org/   턂
> 턢   Mozilla  https://www.mozilla.org/   턂
>  Before I built a wall I'd ask to know
>  What I was walling in or walling out,
>  And to whom I was like to give offense.
>- Robert Frost, Mending Wall (1914)




signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Karl Tomlinson]

Nathan Froyd writes:
> I think a broader definition of "POD struct" would be required here:
> RefPtr and similar are technically not POD, but I *think* you'd
> want to require RefPtr* arguments when you expect the smart pointer
> to be assigned into?  Not sure.

Yes, please, for similar reasons as for the primitive types.

>On Tue, May 9, 2017 at 10:39 AM, Boris Zbarsky  wrote:
>> But for object-typed things like dom::Element or nsIFrame, it seems better
>> to me to pass references instead of pointers (i.e "Element&" vs "Element*")
>> for what are fundamentally in params, even though the callee may call
>> mutators on the passed-in object.  That is, the internal state of the
>> element may change, but its identity will not.

Nathan Froyd writes:
> I get this argument: you want the non-nullability of references, but
> you don't want the verboseness (or whatever) of NonNull or similar,
> so you're using T& as a better T*.  I think I would feel a little
> better about this rule if we permitted it only for types that deleted
> assignment operators.  Not sure if that's really practical to enforce.

With convention to use references to objects for methods that
change the state of the object, the advantages are

R1. Nullability is indicated differently from NotNull.

R2. Parameters which may change the identity of an object can be
distinguished at the call site from those that may change only
state.

With convention to use pointers to objects for methods that change
the state of the object, the advantages are

P1. Parameters which change the state of an object can be
distinguished at the call site from those that don't.

P2. Consistency with primitive types.

My experience is that for understanding while reading code, it is
more helpful to distinguish where state may be modified than to
distinguish what may be null.  One particular case is where a
method transfers state from one object to another.  At the call
site it is useful to distinguish source from destination.

For object types, change in identity is rare (non-existent for
ref-counted objects) and so there is rarely a need to distinguish
this from state change.  Typically identity changes only when the
pointer changes.

Doesn't NotNull provide that pointers can have the same
advantages as references re visibility of nullability where
desired, so we can have the best of both?

Or is NotNull really too awkward IRL?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

RE: Running mochitest on packaged builds with the sandbox

[Kearwood Kip Gilbert]

+cc Diego, who has experimented with packaging Firefox on Steam...

Thanks Alex,

It sounds as though we can work around all of these issues, based on your 
feedback.  I believe that we could proxy any file access needed by the content 
process.  The main process will need to access some resources in these builds 
that are not normally accessed by the normal Firefox builds for the Steam 
integration.  Perhaps opening these resources isn’t too much concern for the 
regular Firefox builds.  If not, we could perhaps select these alternate 
sandboxing rules with a command-line parameter?

We have also explored treating this “Steam VR Browser” as a separate product, 
and using compile-time flags to select the desired code to build and assets.  
We could still hard-code the rules, but skip them at build time for regular 
Firefox builds.  In this case, we are effectively forking the “browser” 
directory into a “vrbrowser” and stripping it down to just what we need.

We liked the re-pack option (ie. Similar to QBRT), as we don’t have to put as 
much load on our test infrastructure; however, this is still an option on the 
table if a dedicated build and the associated costs is justified.

Cheers,
- Kearwood “Kip” Gilbert

From: Alex Gaynor
Sent: May 9, 2017 7:58 AM
To: Kearwood Kip Gilbert
Cc: dev-platform@lists.mozilla.org
Subject: Re: Running mochitest on packaged builds with the sandbox

Hi,
Hmm, VR appears to be an interesting challenge.

To expand a bit more on why mochitest+sandboxing is a challenge for packaged 
builds: The way mochitest is set up is that there's a configuration file which 
points to JS files to be loaded for tests. These are loaded by the content 
process. This works ok in non-packaged builds, because in those builds we allow 
read access to the entire source directory ("topsrcdir"); in packaged builds, 
we don't have this whitelist -- we only allow read access inside of the .app 
(to use the macOS lingo), so essentially content is trying to open a random JS 
file, and is blocked.
With that context, disabling sandboxing might be one option, another is for 
your repack to include the mochitest JS files inside the packaged build, then 
everything can work ok. We don't want to do this for general packaged builds 
because there's no reason for SuperPowers.js (for example) to be in a shipped 
FF, but if you're doing a special pack for testing it might make sense.
Does that make sense?
For your other question, about configuration of sandboxing rules. Right now we 
have the ability to have multiple different sets of sandbox rules, for example 
plugin processes and content processes have different sandbox rules, and so 
will GPU processes soon. With that said, it sounds like what you're talking 
about is really in the content process -- for that case, you're really better 
off remoting access to these resources through the parent process. This keeps 
us from expanding the attack surface in content (which is most exposed to the 
dangerous web), right now we're doing all we can to restrict this, so I 
wouldn't be excited about opening it back up :-)
Cheers,
Alex

On Mon, May 8, 2017 at 2:14 PM, Kearwood Kip Gilbert  
wrote:
Hi all,
 
The VR team is working on a Steam packaged browser with a VR specific UI and 
richer VR experience.  In order to prevent the overhead of having the VR 
specific assets included in every Firefox build while still running on tested 
executables, we will be doing a repack build.
 
WebVR will still continue to be supported in the regular Firefox builds; API 
surface area is the same in normal desktop builds.  Mochitests validating these 
API calls should be unaffected.
 
We will need a means for testing the VR frontend and assets that are added with 
the re-pack.  Ideally, we could use the existing test mechanisms, including 
mochitests.
 
Perhaps we could disable the sandbox for these front-end tests?
 
The Steam packaged builds will also need slightly expanded access to resources 
such as files, registry, and pipes required for communication with Steam.
 
Are there any plans to make the sandboxing rules configurable at runtime?
 
Cheers,
• Kearwood “Kip” Gilbert
 
 
From: Alex Gaynor
Sent: May 8, 2017 10:26 AM
To: dev-platform@lists.mozilla.org
Subject: Running mochitest on packaged builds with the sandbox
 
Hi dev-platform,
 
Top-line question: Do you rely on being able to run mochitests from a
packaged build (`--appname`)?
 
Context:
 
The sandboxing team has been hard at work making the content process
sandbox as restrictive as possible. Our latest focus is  removing file read
permissions from content processes -- the sandbox's value is pretty limited
if a compromised content process can ship all your files off by itself!
 
One of the things we've discovered in the process of developing these
patches is that they break running mochitest on packaged firefox builds
(this is the `--appname` flag to mochitest)! `try` doesn't appear to use
this, and 

Re: Using references vs. pointers in C++ code

[Michael Layzell]

On Tue, May 9, 2017 at 12:05 PM, Boris Zbarsky  wrote:

> On 5/9/17 11:41 AM, Nathan Froyd wrote:
>
>> I think I would feel a little
>> better about this rule if we permitted it only for types that deleted
>> assignment operators.  Not sure if that's really practical to enforce.
>>
>
> Hmm.  I wonder what happens right now if you try to invoke
> nsINode::operator=
>
> But yes, having such a restriction would make sense to me if we can do it.


It wouldn't be too difficult to have a static analysis which complains if a
RefCounted object (An object with AddRef and Release methods) is assigned
into at all, with the assumption that that will do The Wrong Thing™ 100% of
the time.

This of course would only be caught on infrastructure because most people
don't run the SA locally (we have a bug to make this easier which Andi is
working on), but that might be OK as none of this code would get into the
tree.


>
>
> -Boris
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[L. David Baron]

On Tuesday 2017-05-09 11:58 +0200, Emilio Cobos Álvarez wrote:
> I think references help to encode that a bit more in the type system,
> and help reasoning about the code without having to look at the
> implementation of the function you're calling into, or without having to
> rely on the callers to know that you expect a non-null argument.
> 
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.

I have two general concerns about references:

 (1) When reading the calling code that calls a function taking a
 non-const reference, it's not obvious that the function might
 mutate the value, particularly in code that uses references
 rarely, or for types that are rarely passed as references.

 (2) Code that mixes functions that take references and functions
 that take pointers, for the same time, is a mess, because you
 constantly have to worry about whether to write * or &.  Thus,
 in Gecko, we generally use pointers or references per-type.
 XPCOM interfaces are passed by pointer.  String classes are
 passed by reference.  etc.

I think per-type conventions for using references versus pointers
helps with both (1) and (2), and I consider it part of Gecko style
even if it's not well-documented.

I'm also not sure how much references help with enforcing
non-null-ness.  People will just write a "*" whether or not their
existing code guarantees a non-null pointer; at least that was my
experience in the early days of Gecko.  Perhaps we have better code
review now... but I'm somewhat skeptical of that given that I think
we've been relying on tests more and review less?

-David

-- 
턞   L. David Baron http://dbaron.org/   턂
턢   Mozilla  https://www.mozilla.org/   턂
 Before I built a wall I'd ask to know
 What I was walling in or walling out,
 And to whom I was like to give offense.
   - Robert Frost, Mending Wall (1914)


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Running mochitest on packaged builds with the sandbox

[Gian-Carlo Pascutto]

On 08-05-17 19:26, Alex Gaynor wrote:
> Hi dev-platform,
> 
> Top-line question: Do you rely on being able to run mochitests from a
> packaged build (`--appname`)?

It seems our Linux tests do, actually:

https://treeherder.mozilla.org/logviewer.html#?job_id=97391302=try

"test-linux32/opt-mochitest-e10s-1 tc-M-e10s(1)" launches

/home/worker/workspace/build/tests/mochitest/runtests.py --total-chunks
10 --this-chunk 1
--appname=/home/worker/workspace/build/application/firefox/firefox
--utility-path=tests/bin --extra-profile-file=tests/bin/plugins

Using --appname. As expected, this then fails because the sandbox didn't
get the exception added and correctly blocks the reading of some test
files that are in a random place:

/home/worker/workspace/build/tests/mochitest/extensions/specialpowers/chrome/specialpowers/content/MozillaLogger.js

We may need another solution here.

-- 
GCP
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Boris Zbarsky]

On 5/9/17 11:41 AM, Nathan Froyd wrote:

I think I would feel a little
better about this rule if we permitted it only for types that deleted
assignment operators.  Not sure if that's really practical to enforce.


Hmm.  I wonder what happens right now if you try to invoke 
nsINode::operator=


But yes, having such a restriction would make sense to me if we can do it.

-Boris

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Emilio Cobos Álvarez]

On Tue, May 09, 2017 at 06:24:56PM +0300, smaug wrote:
> On 05/09/2017 04:52 PM, smaug wrote:
> > On 05/09/2017 01:55 PM, Mike Hommey wrote:
> > > On Tue, May 09, 2017 at 01:31:33PM +0300, Henri Sivonen wrote:
> > > > On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez 
> > > >  wrote:
> > > > > I think references help to encode that a bit more in the type system,
> > > > > and help reasoning about the code without having to look at the
> > > > > implementation of the function you're calling into, or without having 
> > > > > to
> > > > > rely on the callers to know that you expect a non-null argument.
> > > > > 
> > > > > Personally, I don't think that the fact that they're not used as much 
> > > > > as
> > > > > they could/should is a good argument to prevent their usage, but I 
> > > > > don't
> > > > > know what's the general opinion on that.
> > > > 
> > > > The relevant bit of the Core Guidelines is
> > > > https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref
> > > > and says:
> > > > "A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
> > > > no valid "null reference". Sometimes having nullptr as an alternative
> > > > to indicated "no object" is useful, but if it is not, a reference is
> > > > notationally simpler and might yield better code."
> > > > 
> > > > As a result, I have an in-flight patch that takes T& instead of
> > > > NotNull where applicable, even though I do use NotNull to
> > > > annotate return values.
> > > > 
> > > > I agree that in principle it makes sense to use the type system
> > > > instead of relying on partial debug-build run-time measures to denote
> > > > non-null arguments when possible. That said, having to dereference a
> > > > smart pointer with prefix * in order to pass its referent to a method
> > > > that takes a T& argument feels a bit odd when one is logically
> > > > thinking of passing a pointer still, but then, again, &*foo seems like
> > > > common pattern on the Rust side of FFI to make a reference out of a
> > > > pointer and effectively asserting to the human reader that the pointer
> > > > is null.
> > > 
> > > Note that if you dereference a pointer to pass as a reference, all hell
> > > breaks loose and your reference might just as well be null, but the
> > > function taking the reference won't be protected against it because the
> > > compiler will have assumed, when compiling it, that the reference can't
> > > be null.
> > > 
> > This is what I'm a bit worried. We're moving some null checks from callee 
> > to caller, so need to
> > learn to be more careful with null checks on caller side.
> > But in general, using references sounds ok.
> > 
> 
> 
> I could add still, that using references does add another thing to reviewers' 
> checklist to ensure
> that null pointer isn't ever dereferenced, and it also means that there will 
> be more null checks, since
> parameters need to be validated on callers' side, not in callee.

To be clear, I'm not suggesting to move functions that now do the
null-checking in the function itself to take references. I think
functions that take pointers and handle null are perfectly fine.

But doing functions that already don't null-check because they're
already checked at the callsite seem to me they could perfectly take a
reference.

Also, there's a case for the opposite I think, where taking references
make reviewing easier.

To give one example, I just grepped a bit and found
EffectCompositor::GetAnimationElementAndPseudoForFrame[1] (there are a
lot of other functions like that, like all the IsFoo functions in the
frame constructor, etc).

In any case: That function dereferences the aFrame parameter without
null-checking, not even asserting.

If someone is, hypothetically, adding a caller to that function, and
calls it with a frame pointer that may be null, I think that function
taking a reference would, in any case, make the job of the reviewer
_easier_: It makes the explicit dereference at the callsite, so the
reviewer can evaluate whether that dereference is correct.

Otherwise, the reviewer would need to check the implementation of
GetAnimationElementAndPseudoForFrame in order to realise that it doesn't
handle null.

It's kind of a silly example (because that function itself doesn't make
a lot of sense if you don't give it a frame), but I can find some more
realistic examples of the same situation if you want to.

  -- Emilio

[1]: 
http://searchfox.org/mozilla-central/source/dom/animation/EffectCompositor.cpp#704


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Running mochitest on packaged builds with the sandbox

[Nathan Froyd]

On Mon, May 8, 2017 at 1:26 PM, Alex Gaynor  wrote:
> Top-line question: Do you rely on being able to run mochitests from a
> packaged build (`--appname`)?

I don't think it's a *fundamental* part of development workflows, but
I know folks have found value in being able to run tests--including
but not limited to mochitest--against packaged builds (release
versions, beta versions, whatever).  It would be nice to not break
that, or at least provide obvious escape hatches where possible.

-Nathan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Nathan Froyd]

On Tue, May 9, 2017 at 10:39 AM, Boris Zbarsky  wrote:
> On 5/9/17 9:17 AM, Nathan Froyd wrote:
>> The argument I have always heard, Gecko-wise and elsewhere [1], is to
>> prefer pointers for modification
>
> This is for primitive-typed out or inout params, right?

I don't remember hearing any distinction one way or the other.  I
don't think we have a rule written down for Gecko, but a lot of the
things we have historically dealt with are heap-allocated anyway or
have had to extensively deal with XPIDL, so passing around pointers
has seemed natural.  But many recent things (e.g. WebIDL, some of the
editor refactorings, etc.) have started to prefer references even to
heap-allocated things, so we now have to think a little harder.

> In other words, we should prefer "int*" to "int&" for places where we expect
> the callee to modify the int, just like we should prefer "MyClass**" to
> "MyClass*&".  I guess the same for POD structs if we expect people to be
> writing to them wholesale via assignment operators? Not sure.

I think a broader definition of "POD struct" would be required here:
RefPtr and similar are technically not POD, but I *think* you'd
want to require RefPtr* arguments when you expect the smart pointer
to be assigned into?  Not sure.

> But for object-typed things like dom::Element or nsIFrame, it seems better
> to me to pass references instead of pointers (i.e "Element&" vs "Element*")
> for what are fundamentally in params, even though the callee may call
> mutators on the passed-in object.  That is, the internal state of the
> element may change, but its identity will not.

I get this argument: you want the non-nullability of references, but
you don't want the verboseness (or whatever) of NonNull or similar,
so you're using T& as a better T*.  I think I would feel a little
better about this rule if we permitted it only for types that deleted
assignment operators.  Not sure if that's really practical to enforce.

-Nathan
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Running mochitest on packaged builds with the sandbox

[Ehsan Akhgari]

On 05/09/2017 11:02 AM, Alex Gaynor wrote:

Hi Ehsan,

If we want to dig deeper, let's fork off another thread, but it sounds 
like there's two action items here:


1) Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1345046
2) Better document how to disable the sandbox for debugging -- where 
would you expect to find docs on this, 
https://wiki.mozilla.org/Security/Sandbox, somewhere else?


I really think we should do #1 if at all possible.  If that's not an 
option, I think we should print out something helpful to stderr when we 
see the logging environment variables pointing to a file name in a 
sandboxed content process, the problem with picking a wiki page like 
above is that most people won't immediately realize that it's sandboxing 
that makes the log files not get generated and start wasting time 
debugging things until they get to that conclusion and it is only then 
when they start to search for relevant documentation in a place related 
to sandboxing.


Now let's go back to discussing the actual topic of the thread. Thanks 
for indulging the momentary digression from the topic at hand.  :-)


Cheers,
Ehsan


Cheers,
Alex

On Tue, May 9, 2017 at 10:49 AM, Ehsan Akhgari 
> wrote:


Hi Alex,

Apologies for hijacking the thread, but since you asked, right now
debugging mochitest that you want to get some logging out of with
a sandboxed content process is super painful.  I last hit it when
I was debugging a memory leak which typically requires getting
refcount leak logs and it took me quite a while to find the wiki
page that describes the pref that I needed to set in order to turn
off the sandbox so that any logging in the content process would
be able to write to a log file (and I couldn't even find it again
to include a link to the wiki page here once again!).

I thought I'd mention it since you were asking about stuff that
can be painful when debugging test failures with sandboxed content
processes.  :-)

Thanks,

Ehsan



On 05/08/2017 01:26 PM, Alex Gaynor wrote:

Hi dev-platform,

Top-line question: Do you rely on being able to run mochitests
from a
packaged build (`--appname`)?

Context:

The sandboxing team has been hard at work making the content
process
sandbox as restrictive as possible. Our latest focus is 
removing file read

permissions from content processes -- the sandbox's value is
pretty limited
if a compromised content process can ship all your files off
by itself!

One of the things we've discovered in the process of
developing these
patches is that they break running mochitest on packaged
firefox builds
(this is the `--appname` flag to mochitest)! `try` doesn't
appear to use
this, and none of us use it in our development workflows, but
we wanted to
check in with dev-platform and see if we were going to be
breaking people's
development flows! While these restrictions are not on by
default yet, once
they are you'd only be able to run tests on packaged builds by
disabling
the sandbox. If this is a fundamental part of lots of folks'
workflows
we'll dig into whether there's a way to keep this working.

Happy Monday!
Alex
___
dev-platform mailing list
dev-platform@lists.mozilla.org

https://lists.mozilla.org/listinfo/dev-platform



___
dev-platform mailing list
dev-platform@lists.mozilla.org 
https://lists.mozilla.org/listinfo/dev-platform





___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[smaug]

On 05/09/2017 04:52 PM, smaug wrote:

On 05/09/2017 01:55 PM, Mike Hommey wrote:

On Tue, May 09, 2017 at 01:31:33PM +0300, Henri Sivonen wrote:

On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez  wrote:

I think references help to encode that a bit more in the type system,
and help reasoning about the code without having to look at the
implementation of the function you're calling into, or without having to
rely on the callers to know that you expect a non-null argument.

Personally, I don't think that the fact that they're not used as much as
they could/should is a good argument to prevent their usage, but I don't
know what's the general opinion on that.


The relevant bit of the Core Guidelines is
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref
and says:
"A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
no valid "null reference". Sometimes having nullptr as an alternative
to indicated "no object" is useful, but if it is not, a reference is
notationally simpler and might yield better code."

As a result, I have an in-flight patch that takes T& instead of
NotNull where applicable, even though I do use NotNull to
annotate return values.

I agree that in principle it makes sense to use the type system
instead of relying on partial debug-build run-time measures to denote
non-null arguments when possible. That said, having to dereference a
smart pointer with prefix * in order to pass its referent to a method
that takes a T& argument feels a bit odd when one is logically
thinking of passing a pointer still, but then, again, &*foo seems like
common pattern on the Rust side of FFI to make a reference out of a
pointer and effectively asserting to the human reader that the pointer
is null.


Note that if you dereference a pointer to pass as a reference, all hell
breaks loose and your reference might just as well be null, but the
function taking the reference won't be protected against it because the
compiler will have assumed, when compiling it, that the reference can't
be null.


This is what I'm a bit worried. We're moving some null checks from callee to 
caller, so need to
learn to be more careful with null checks on caller side.
But in general, using references sounds ok.




I could add still, that using references does add another thing to reviewers' 
checklist to ensure
that null pointer isn't ever dereferenced, and it also means that there will be 
more null checks, since
parameters need to be validated on callers' side, not in callee.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Running mochitest on packaged builds with the sandbox

[Alex Gaynor]

Hi Ehsan,

If we want to dig deeper, let's fork off another thread, but it sounds like
there's two action items here:

1) Fix https://bugzilla.mozilla.org/show_bug.cgi?id=1345046
2) Better document how to disable the sandbox for debugging -- where would
you expect to find docs on this, https://wiki.mozilla.org/Security/Sandbox,
somewhere else?

Cheers,
Alex

On Tue, May 9, 2017 at 10:49 AM, Ehsan Akhgari 
wrote:

> Hi Alex,
>
> Apologies for hijacking the thread, but since you asked, right now
> debugging mochitest that you want to get some logging out of with a
> sandboxed content process is super painful.  I last hit it when I was
> debugging a memory leak which typically requires getting refcount leak logs
> and it took me quite a while to find the wiki page that describes the pref
> that I needed to set in order to turn off the sandbox so that any logging
> in the content process would be able to write to a log file (and I couldn't
> even find it again to include a link to the wiki page here once again!).
>
> I thought I'd mention it since you were asking about stuff that can be
> painful when debugging test failures with sandboxed content processes.  :-)
>
> Thanks,
>
> Ehsan
>
>
>
> On 05/08/2017 01:26 PM, Alex Gaynor wrote:
>
>> Hi dev-platform,
>>
>> Top-line question: Do you rely on being able to run mochitests from a
>> packaged build (`--appname`)?
>>
>> Context:
>>
>> The sandboxing team has been hard at work making the content process
>> sandbox as restrictive as possible. Our latest focus is  removing file
>> read
>> permissions from content processes -- the sandbox's value is pretty
>> limited
>> if a compromised content process can ship all your files off by itself!
>>
>> One of the things we've discovered in the process of developing these
>> patches is that they break running mochitest on packaged firefox builds
>> (this is the `--appname` flag to mochitest)! `try` doesn't appear to use
>> this, and none of us use it in our development workflows, but we wanted to
>> check in with dev-platform and see if we were going to be breaking
>> people's
>> development flows! While these restrictions are not on by default yet,
>> once
>> they are you'd only be able to run tests on packaged builds by disabling
>> the sandbox. If this is a fundamental part of lots of folks' workflows
>> we'll dig into whether there's a way to keep this working.
>>
>> Happy Monday!
>> Alex
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
>>
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Running mochitest on packaged builds with the sandbox

[Alex Gaynor]

Hi,

Hmm, VR appears to be an interesting challenge.

To expand a bit more on why mochitest+sandboxing is a challenge for
packaged builds: The way mochitest is set up is that there's a
configuration file which points to JS files to be loaded for tests. These
are loaded by the content process. This works ok in non-packaged builds,
because in those builds we allow read access to the entire source directory
("topsrcdir"); in packaged builds, we don't have this whitelist -- we only
allow read access inside of the .app (to use the macOS lingo), so
essentially content is trying to open a random JS file, and is blocked.

With that context, disabling sandboxing might be one option, another is for
your repack to include the mochitest JS files inside the packaged build,
then everything can work ok. We don't want to do this for general packaged
builds because there's no reason for SuperPowers.js (for example) to be in
a shipped FF, but if you're doing a special pack for testing it might make
sense.

Does that make sense?

For your other question, about configuration of sandboxing rules. Right now
we have the ability to have multiple different sets of sandbox rules, for
example plugin processes and content processes have different sandbox
rules, and so will GPU processes soon. With that said, it sounds like what
you're talking about is really in the content process -- for that case,
you're really better off remoting access to these resources through the
parent process. This keeps us from expanding the attack surface in content
(which is most exposed to the dangerous web), right now we're doing all we
can to restrict this, so I wouldn't be excited about opening it back up :-)

Cheers,
Alex

On Mon, May 8, 2017 at 2:14 PM, Kearwood Kip Gilbert 
wrote:

> Hi all,
>
>
>
> The VR team is working on a Steam packaged browser with a VR specific UI
> and richer VR experience.  In order to prevent the overhead of having the
> VR specific assets included in every Firefox build while still running on
> tested executables, we will be doing a repack build.
>
>
>
> WebVR will still continue to be supported in the regular Firefox builds;
> API surface area is the same in normal desktop builds.  Mochitests
> validating these API calls should be unaffected.
>
>
>
> We will need a means for testing the VR frontend and assets that are added
> with the re-pack.  Ideally, we could use the existing test mechanisms,
> including mochitests.
>
>
>
> Perhaps we could disable the sandbox for these front-end tests?
>
>
>
> The Steam packaged builds will also need slightly expanded access to
> resources such as files, registry, and pipes required for communication
> with Steam.
>
>
>
> Are there any plans to make the sandboxing rules configurable at runtime?
>
>
>
> Cheers,
>
>- Kearwood “Kip” Gilbert
>
>
>
>
>
> *From: *Alex Gaynor 
> *Sent: *May 8, 2017 10:26 AM
> *To: *dev-platform@lists.mozilla.org
> *Subject: *Running mochitest on packaged builds with the sandbox
>
>
>
> Hi dev-platform,
>
>
>
> Top-line question: Do you rely on being able to run mochitests from a
>
> packaged build (`--appname`)?
>
>
>
> Context:
>
>
>
> The sandboxing team has been hard at work making the content process
>
> sandbox as restrictive as possible. Our latest focus is  removing file read
>
> permissions from content processes -- the sandbox's value is pretty limited
>
> if a compromised content process can ship all your files off by itself!
>
>
>
> One of the things we've discovered in the process of developing these
>
> patches is that they break running mochitest on packaged firefox builds
>
> (this is the `--appname` flag to mochitest)! `try` doesn't appear to use
>
> this, and none of us use it in our development workflows, but we wanted to
>
> check in with dev-platform and see if we were going to be breaking people's
>
> development flows! While these restrictions are not on by default yet, once
>
> they are you'd only be able to run tests on packaged builds by disabling
>
> the sandbox. If this is a fundamental part of lots of folks' workflows
>
> we'll dig into whether there's a way to keep this working.
>
>
>
> Happy Monday!
>
> Alex
>
> ___
>
> dev-platform mailing list
>
> dev-platform@lists.mozilla.org
>
> https://lists.mozilla.org/listinfo/dev-platform
>
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Running mochitest on packaged builds with the sandbox

[Ehsan Akhgari]

Hi Alex,

Apologies for hijacking the thread, but since you asked, right now 
debugging mochitest that you want to get some logging out of with a 
sandboxed content process is super painful.  I last hit it when I was 
debugging a memory leak which typically requires getting refcount leak 
logs and it took me quite a while to find the wiki page that describes 
the pref that I needed to set in order to turn off the sandbox so that 
any logging in the content process would be able to write to a log file 
(and I couldn't even find it again to include a link to the wiki page 
here once again!).


I thought I'd mention it since you were asking about stuff that can be 
painful when debugging test failures with sandboxed content processes.  :-)


Thanks,

Ehsan


On 05/08/2017 01:26 PM, Alex Gaynor wrote:

Hi dev-platform,

Top-line question: Do you rely on being able to run mochitests from a
packaged build (`--appname`)?

Context:

The sandboxing team has been hard at work making the content process
sandbox as restrictive as possible. Our latest focus is  removing file read
permissions from content processes -- the sandbox's value is pretty limited
if a compromised content process can ship all your files off by itself!

One of the things we've discovered in the process of developing these
patches is that they break running mochitest on packaged firefox builds
(this is the `--appname` flag to mochitest)! `try` doesn't appear to use
this, and none of us use it in our development workflows, but we wanted to
check in with dev-platform and see if we were going to be breaking people's
development flows! While these restrictions are not on by default yet, once
they are you'd only be able to run tests on packaged builds by disabling
the sandbox. If this is a fundamental part of lots of folks' workflows
we'll dig into whether there's a way to keep this working.

Happy Monday!
Alex
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform


___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Boris Zbarsky]

On 5/9/17 9:03 AM, Bobby Holley wrote:

I think passing non-nullable things by reference is good, but I think we
should keep it consistent for a given type.


I should note that we already have this across all types to some extent: 
WebIDL bindings pass non-nullable interface types as references, on 
purpose, to make it clear that they can't be null (and conversely to 
make it very clear which things _can_ be null and hence need null-checking).


In case it's not obvious, I support passing non-nullable things as 
references.  ;)


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Boris Zbarsky]

On 5/9/17 9:17 AM, Nathan Froyd wrote:

On Tue, May 9, 2017 at 5:58 AM, Emilio Cobos Álvarez  wrote:

Personally, I don't think that the fact that they're not used as much as
they could/should is a good argument to prevent their usage, but I don't
know what's the general opinion on that.


The argument I have always heard, Gecko-wise and elsewhere [1], is to
prefer pointers for modification


This is for primitive-typed out or inout params, right?

In other words, we should prefer "int*" to "int&" for places where we 
expect the callee to modify the int, just like we should prefer 
"MyClass**" to "MyClass*&".  I guess the same for POD structs if we 
expect people to be writing to them wholesale via assignment operators? 
Not sure.


But for object-typed things like dom::Element or nsIFrame, it seems 
better to me to pass references instead of pointers (i.e "Element&" vs 
"Element*") for what are fundamentally in params, even though the callee 
may call mutators on the passed-in object.  That is, the internal state 
of the element may change, but its identity will not.


-Boris
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[smaug]

On 05/09/2017 01:55 PM, Mike Hommey wrote:

On Tue, May 09, 2017 at 01:31:33PM +0300, Henri Sivonen wrote:

On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez  wrote:

I think references help to encode that a bit more in the type system,
and help reasoning about the code without having to look at the
implementation of the function you're calling into, or without having to
rely on the callers to know that you expect a non-null argument.

Personally, I don't think that the fact that they're not used as much as
they could/should is a good argument to prevent their usage, but I don't
know what's the general opinion on that.


The relevant bit of the Core Guidelines is
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref
and says:
"A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
no valid "null reference". Sometimes having nullptr as an alternative
to indicated "no object" is useful, but if it is not, a reference is
notationally simpler and might yield better code."

As a result, I have an in-flight patch that takes T& instead of
NotNull where applicable, even though I do use NotNull to
annotate return values.

I agree that in principle it makes sense to use the type system
instead of relying on partial debug-build run-time measures to denote
non-null arguments when possible. That said, having to dereference a
smart pointer with prefix * in order to pass its referent to a method
that takes a T& argument feels a bit odd when one is logically
thinking of passing a pointer still, but then, again, &*foo seems like
common pattern on the Rust side of FFI to make a reference out of a
pointer and effectively asserting to the human reader that the pointer
is null.


Note that if you dereference a pointer to pass as a reference, all hell
breaks loose and your reference might just as well be null, but the
function taking the reference won't be protected against it because the
compiler will have assumed, when compiling it, that the reference can't
be null.


This is what I'm a bit worried. We're moving some null checks from callee to 
caller, so need to
learn to be more careful with null checks on caller side.
But in general, using references sounds ok.


-Olli
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Nathan Froyd]

On Tue, May 9, 2017 at 5:58 AM, Emilio Cobos Álvarez  wrote:
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.

The argument I have always heard, Gecko-wise and elsewhere [1], is to
prefer pointers for modification, because it's clearly signaled at the
callsite that something might be happening to the value.  That would
rule out `T&`, but permit `const T&`.

-Nathan

[1] https://google.github.io/styleguide/cppguide.html#Reference_Arguments
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Eric Rescorla]

As Henri indicates, I think the use of references is consistent with
the style guide.

It's also worth noting that if you are using boxed pointers, then you
almost certainly want to use references to pass them around. I.e.,

foo(const RefPtr& mPtr);   // avoids useless ref count
foo(const UniquePtr& mPtr); // avoids attempted (and failed)
ownership transfer.


-Ekr

P.S. The Google style guide prohibits non-const references. We follow this
in the parts of the code where we follow that style (e.g., the NSS
gtests).
https://google.github.io/styleguide/cppguide.html#Reference_Arguments


On Tue, May 9, 2017 at 3:31 AM, Henri Sivonen  wrote:

> On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez 
> wrote:
> > I think references help to encode that a bit more in the type system,
> > and help reasoning about the code without having to look at the
> > implementation of the function you're calling into, or without having to
> > rely on the callers to know that you expect a non-null argument.
> >
> > Personally, I don't think that the fact that they're not used as much as
> > they could/should is a good argument to prevent their usage, but I don't
> > know what's the general opinion on that.
>
> The relevant bit of the Core Guidelines is
> https://github.com/isocpp/CppCoreGuidelines/blob/master/
> CppCoreGuidelines.md#Rf-ptr-ref
> and says:
> "A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
> no valid "null reference". Sometimes having nullptr as an alternative
> to indicated "no object" is useful, but if it is not, a reference is
> notationally simpler and might yield better code."
>
> As a result, I have an in-flight patch that takes T& instead of
> NotNull where applicable, even though I do use NotNull to
> annotate return values.
>
> I agree that in principle it makes sense to use the type system
> instead of relying on partial debug-build run-time measures to denote
> non-null arguments when possible. That said, having to dereference a
> smart pointer with prefix * in order to pass its referent to a method
> that takes a T& argument feels a bit odd when one is logically
> thinking of passing a pointer still, but then, again, &*foo seems like
> common pattern on the Rust side of FFI to make a reference out of a
> pointer and effectively asserting to the human reader that the pointer
> is null.
>
> --
> Henri Sivonen
> hsivo...@hsivonen.fi
> https://hsivonen.fi/
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Bobby Holley]

I think passing non-nullable things by reference is good, but I think we
should keep it consistent for a given type. So, for example, we shouldn't
have some callsites that take an nsPresContext* and others that take an
nsPresContext& (unless we have a rare case of a nullable presContext arg,
in which case |nsPresContext* aPresContextOrNull| would suffice).

Basically, I think consistency is important, but maintaining that
consistency per-type rather than system-wide seems like a reasonable
compromise. Curious as to what others think here.

bholley

On Tue, May 9, 2017 at 11:58 AM, Emilio Cobos Álvarez 
wrote:

> Hi dev-platform@,
>
> So, yesterday was working on a bug (bug 1362991, if you're curious) when
> I decided to do some spring cleanup and pass some non-optional argument
> as a reference instead of as a pointer.
>
> I got the cleanup patch rejected, because it went against the prevailing
> style of the codebase, and I was told that moving to references required
> a discussion in dev-platform, resulting in something more explicit in
> the style guide.
>
> So, I'll revert that patch for now, but I'm sending this mail to
> kickstart that discussion :)
>
> TL;DR: I think references are useful. We have encoded nullability of
> arguments and return values implicitly or explicitly in different ways:
> assertions inside of the function, naming conventions...
>
> I think references help to encode that a bit more in the type system,
> and help reasoning about the code without having to look at the
> implementation of the function you're calling into, or without having to
> rely on the callers to know that you expect a non-null argument.
>
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.
>
> Thanks for reading.
>
>  -- Emilio
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Mixing nsresult and Result code

[Nicolas B. Pierron]

On 05/07/2017 07:34 PM, Kris Maglione wrote:

  static inline Result
  WrapNSResult(PRStatus aRv)
  {
  if (aRv != PR_SUCCESS) {
  return Err(NS_ERROR_FAILURE);
  }
  return Ok();
  }

  static inline Result
  WrapNSResult(nsresult aRv)
  {
  if (NS_FAILED(aRv)) {
  return Err(aRv);
  }
  return Ok();
  }

  #define NS_TRY(expr) MOZ_TRY(WrapNSResult(expr))


This sounds like we could have a template function named ToResult(), which 
is used by the MOZ_TRY macro to coerce a given type into the equivalent 
mozilla::Result<> type.  This ToResult function could be specialized to 
forward the Result<…> arguments by default.


 #define MOZ_TRY(expr) \
   do { \
-auto mozTryTempResult_ = (expr); \
+auto mozTryTempResult_ = ::mozilla::ToResult(expr); \
 if (mozTryTempResult_.isErr()) { \
   return ::mozilla::Err(mozTryTempResult_.unwrapErr()); \
 } \
   } while (0)

Another concern would be the representation of the Result (or 
Result ?), which should definitely be represented as 
an nsresult, as this enum is capable of representing success with NS_OK, and 
also various other forms of success values.  There is already a way to 
specialize the PackingStrategy of the Result class to consume less space, 
and it can be used to specialize the way we encode such Result 
class, to avoid the overhead of duplicating the success flag.


--
Nicolas B. Pierron
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Mike Hommey]

On Tue, May 09, 2017 at 01:31:33PM +0300, Henri Sivonen wrote:
> On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez  
> wrote:
> > I think references help to encode that a bit more in the type system,
> > and help reasoning about the code without having to look at the
> > implementation of the function you're calling into, or without having to
> > rely on the callers to know that you expect a non-null argument.
> >
> > Personally, I don't think that the fact that they're not used as much as
> > they could/should is a good argument to prevent their usage, but I don't
> > know what's the general opinion on that.
> 
> The relevant bit of the Core Guidelines is
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref
> and says:
> "A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
> no valid "null reference". Sometimes having nullptr as an alternative
> to indicated "no object" is useful, but if it is not, a reference is
> notationally simpler and might yield better code."
> 
> As a result, I have an in-flight patch that takes T& instead of
> NotNull where applicable, even though I do use NotNull to
> annotate return values.
> 
> I agree that in principle it makes sense to use the type system
> instead of relying on partial debug-build run-time measures to denote
> non-null arguments when possible. That said, having to dereference a
> smart pointer with prefix * in order to pass its referent to a method
> that takes a T& argument feels a bit odd when one is logically
> thinking of passing a pointer still, but then, again, &*foo seems like
> common pattern on the Rust side of FFI to make a reference out of a
> pointer and effectively asserting to the human reader that the pointer
> is null.

Note that if you dereference a pointer to pass as a reference, all hell
breaks loose and your reference might just as well be null, but the
function taking the reference won't be protected against it because the
compiler will have assumed, when compiling it, that the reference can't
be null.

Mike
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Henri Sivonen]

On Tue, May 9, 2017 at 12:58 PM, Emilio Cobos Álvarez  wrote:
> I think references help to encode that a bit more in the type system,
> and help reasoning about the code without having to look at the
> implementation of the function you're calling into, or without having to
> rely on the callers to know that you expect a non-null argument.
>
> Personally, I don't think that the fact that they're not used as much as
> they could/should is a good argument to prevent their usage, but I don't
> know what's the general opinion on that.

The relevant bit of the Core Guidelines is
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Rf-ptr-ref
and says:
"A pointer (T*) can be a nullptr and a reference (T&) cannot, there is
no valid "null reference". Sometimes having nullptr as an alternative
to indicated "no object" is useful, but if it is not, a reference is
notationally simpler and might yield better code."

As a result, I have an in-flight patch that takes T& instead of
NotNull where applicable, even though I do use NotNull to
annotate return values.

I agree that in principle it makes sense to use the type system
instead of relying on partial debug-build run-time measures to denote
non-null arguments when possible. That said, having to dereference a
smart pointer with prefix * in order to pass its referent to a method
that takes a T& argument feels a bit odd when one is logically
thinking of passing a pointer still, but then, again, &*foo seems like
common pattern on the Rust side of FFI to make a reference out of a
pointer and effectively asserting to the human reader that the pointer
is null.

-- 
Henri Sivonen
hsivo...@hsivonen.fi
https://hsivonen.fi/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Using references vs. pointers in C++ code

[Masayuki Nakano]

I like using reference to argument if it's non-nullable.
In Core::Editor module, such arguments are already replaced with 
reference in a lot of places.


--
Masayuki Nakano 
Software Engineer, Mozilla
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Intent to unship: nsIEditorIMESupport

[Masayuki Nakano]

nsIEditorIMESupport is an empty interface. Members were moved to 
nsIEditor and nsIEditorIMESupport was made a subclass of it.


This interface is now completely unused. So, it should be removed from 
the tree completely.


Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1325281

--
Masayuki Nakano 
Software Engineer, Mozilla
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Using references vs. pointers in C++ code

[Emilio Cobos Álvarez]

Hi dev-platform@,

So, yesterday was working on a bug (bug 1362991, if you're curious) when
I decided to do some spring cleanup and pass some non-optional argument
as a reference instead of as a pointer.

I got the cleanup patch rejected, because it went against the prevailing
style of the codebase, and I was told that moving to references required
a discussion in dev-platform, resulting in something more explicit in
the style guide.

So, I'll revert that patch for now, but I'm sending this mail to
kickstart that discussion :)

TL;DR: I think references are useful. We have encoded nullability of
arguments and return values implicitly or explicitly in different ways:
assertions inside of the function, naming conventions...

I think references help to encode that a bit more in the type system,
and help reasoning about the code without having to look at the
implementation of the function you're calling into, or without having to
rely on the callers to know that you expect a non-null argument.

Personally, I don't think that the fact that they're not used as much as
they could/should is a good argument to prevent their usage, but I don't
know what's the general opinion on that.

Thanks for reading.

 -- Emilio


signature.asc
Description: PGP signature
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: CodeCoverage Monthly Update

[Ted Mielczarek]

On Tue, May 9, 2017, at 05:48 AM, Henri Sivonen wrote:
> On Thu, Apr 6, 2017 at 6:26 AM, Kyle Lahnakoski 
> wrote:
> > * Getting Rust to emit coverage artifacts is important:
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1335518
> 
> Is there a plan to factor "cargo test" of individual vendored crates
> into the coverage of Rust code? For example, for encoding_rs, I was
> thinking of testing mainly the C++ integration as an
> mozilla-central-specific gtest and leaving the testing of the crate
> internals to the crate's standalone "cargo test".

Note that we're not currently running `cargo test` tests for anything:
https://bugzilla.mozilla.org/show_bug.cgi?id=1331022

-Ted
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: CodeCoverage Monthly Update

[Henri Sivonen]

On Thu, Apr 6, 2017 at 6:26 AM, Kyle Lahnakoski  wrote:
> * Getting Rust to emit coverage artifacts is important:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1335518

Is there a plan to factor "cargo test" of individual vendored crates
into the coverage of Rust code? For example, for encoding_rs, I was
thinking of testing mainly the C++ integration as an
mozilla-central-specific gtest and leaving the testing of the crate
internals to the crate's standalone "cargo test".

-- 
Henri Sivonen
hsivo...@hsivonen.fi
https://hsivonen.fi/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Intent to unship: MozPhonetic.phonetic (HTMLInputElement.phonetic) and nsIPhonetic

[Masayuki Nakano]

MozPhonetic has an attribute, phonetic, whose type is DOMString. This is 
available only in chrome.

https://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/dom/webidl/HTMLInputElement.webidl#220-224,227

Its XPCOM implementation is nsIPhonetic, which is inherited by 
EditorBase and HTMLInputElement.

https://searchfox.org/mozilla-central/rev/224cc663d54085994a4871ef464b7662e0721e83/dom/html/nsIPhonetic.idl

Nobody (including add-ons) uses them now. So, we can get rid of them.

Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1363278

--
Masayuki Nakano 
Software Engineer, Mozilla
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform