Re: PSA: mozilla::Result is about to understand move semantics

[Bryce Seager van Dyk]

Thanks both for the clarifications.

There is a use case that I'd like to clarify the performance + sanity of. I 
hope discussion here will be useful to others looking to use Result, but please 
let me know if it's more suitable to take elsewhere.

In the Media stack I often want to return a Result. In the Ok case we're passing around potentially 
large pieces of media, in the Err case we have a verbose error that we'd like 
logged. In either case the Result may need to go through several stack frames 
before it's logged or consumed. This seems like an ideal use case for Result to 
me.

In the Err case, it looks like with the up coming changes[0] the error will be 
copied into the Result? I'm not entirely sure I understand the Ok case given 
what was said above.

> But either way, that's going to result in a copy when
> the Result is constructed (unless the compiler is really clever).

Is it the data being moved into the Result which is incurring the copy here, or 
the actual Result that's being returned? I would have thought that the data is 
moved into the Result avoids a copy, then the Result itself would be moved or 
RVOed (either way avoiding a copy).


[0]: https://phabricator.services.mozilla.com/D41425
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: mozilla::Result is about to understand move semantics

[Kris Maglione]

On Mon, Aug 12, 2019 at 11:16:17PM +0200, Emilio Cobos Álvarez wrote:


On 8/12/19 10:38 PM, Bryce Seager van Dyk wrote:

On Monday, August 12, 2019 at 8:41:26 AM UTC-7, Emilio Cobos Álvarez wrote:

Neat! Thanks for doing this. It should allow for broader use of Result in media 
code.

Are there any footguns to watch out for when moving into the Result? Looking at the 
changes it appears that if my return type is Result then I can just 
return a BigStruct? I.e. I don't need to do explicitly invoke anything else to avoid 
unintended copies.


I think you need to use `return std::move(bigStruct);`, since 
otherwise you peek the `const T&` constructor rather than the `T&&` 
constructor. Before my patch, you always get a copy when returning it 
(because Result only took const references).


With my patch, if you `return bigStruct;`, your code will compile and 
do the same copy, but you'll get a warning from the compiler with 
-Wreturn-std-move, such as:


Note that if you construct the returned struct in the return 
statement with something like:


 return BigStruct {
   ...,
 };

it should still invoke the Result<> constructor with move 
semantics. But either way, that's going to result in a copy when 
the Result is constructed (unless the compiler is really clever).


The only way to avoid the copy altogether would be to create the 
result with a default initialized value, fill it in, and then 
return it without an std::move. Which is easier said than done. 
It might make sense to add a `.emplace()` method to make that 
easier...

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

FYI: Taskcluster Services Migration

[Dustin Mitchell]

*tl;dr:* Taskcluster, the platform supporting Firefox CI, will be moving to
a new hosting environment during the tree-closing window at the end of
September.  This is a major change and upgrade to the platform, but may
cause some bumps along the way.

Taskcluster is in the midst of a few migrations [1], and one of those is
moving to a new hosting environment.  This has a few advantages:
 * Operations will be managed by professionals (cloudops), not developers
(TC team)
 * Administration will be managed by professionals (releng), not developers
(TC team)
 * Hosting will be in a more trusted environment (GCP instead of Heroku)
 * New, faster UI
 * New, faster, more reliable worker-provisioning backend

The downside is that URLs will be changing, likely leaving some dangling
pointers.  We've anticipated some of the most common (such as
https://queue.taskcluster.net/ URLs) and are planning workarounds, but
nonetheless anticipate some dead links for a while following the switch.

Dustin

[1]
https://mana.mozilla.org/wiki/pages/viewpage.action?spaceKey=TAS=Migrations
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: mozilla::Result is about to understand move semantics

[Emilio Cobos Álvarez]

On 8/12/19 10:38 PM, Bryce Seager van Dyk wrote:

On Monday, August 12, 2019 at 8:41:26 AM UTC-7, Emilio Cobos Álvarez wrote:

Neat! Thanks for doing this. It should allow for broader use of Result in media 
code.

Are there any footguns to watch out for when moving into the Result? Looking at the 
changes it appears that if my return type is Result then I can just 
return a BigStruct? I.e. I don't need to do explicitly invoke anything else to avoid 
unintended copies.


I think you need to use `return std::move(bigStruct);`, since otherwise 
you peek the `const T&` constructor rather than the `T&&` constructor. 
Before my patch, you always get a copy when returning it (because Result 
only took const references).


With my patch, if you `return bigStruct;`, your code will compile and do 
the same copy, but you'll get a warning from the compiler with 
-Wreturn-std-move, such as:


> In file included from 
z:/build/build/src/obj-firefox/toolkit/mozapps/extensions/Unified_cpp_mozapps_extensions0.cpp:11:
> 
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10): 
error: local variable 'result' will be copied despite being returned by 
name [-Werror,-Wreturn-std-move]

>   return result;
>  ^~
> 
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10): 
note: call 'std::move' explicitly to avoid copying

>   return result;
>  ^~
>  std::move(result)
> 
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10): 
error: local variable 'result' will be copied despite being returned by 
name [-Werror,-Wreturn-std-move]

>   return result;
>  ^~
> 
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(554,20): 
note: in instantiation of function template specialization 
'mozilla::EncodeLZ4' requested here

>   MOZ_TRY_VAR(lz4, EncodeLZ4(scData, STRUCTURED_CLONE_MAGIC));
>^
> 
z:/build/build/src/toolkit/mozapps/extensions/AddonManagerStartup.cpp(197,10): 
note: call 'std::move' explicitly to avoid copying

>   return result;
>  ^~
>  std::move(result)

Note that this does not -Werror everywhere on automation right now[1], 
but I think we should make it so, per that LLVM bug.


But anyhow the compiler will tell you to do the fast thing once my patch 
lands.


 -- Emilio

[1]: 
https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/build/moz.configure/warnings.configure#144




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: PSA: mozilla::Result is about to understand move semantics

[Bryce Seager van Dyk]

On Monday, August 12, 2019 at 8:41:26 AM UTC-7, Emilio Cobos Álvarez wrote:
> If you don't use mozilla::Result, then you can stop reading now.
> 
> Result always returned the ok / errors by value. That's fine when the ok 
> or error values are cheap / small, like integers, but over time we've 
> been using Result> / Result / Result, etc.
> 
> Furthermore Result never worked with move-only types, so you couldn't 
> use Result> or such.
> 
> In bug 1418624 I'm about to change mozilla::Result so that:
> 
>   * unwrap() / unwrapErr() / unwrapOr() / map() / andThen() move rather 
> than copy the result.
> 
>   * inspect() and inspectErr() return a reference to the ok / error 
> values respectively.
> 
> The first change allows the usage of move-only types, and is closer to 
> the Rust semantics (though C++ doesn't have the borrow-checker to 
> prevent use after moves).
> 
> The second change allows having big Result<>s on the stack, while not 
> paying extra stack space to inspect them.
> 
> I audited and changed all callers that were calling the now-moving 
> methods multiple times on the same error to use inspect() / 
> inspectErr(). Most of them shouldn't matter, as moving trivial types 
> doesn't do anything, but for some cases like some usages of std::wstring 
> it should have a positive impact on performance.
> 
> Let me know if there's any concerns with this or what not.
> 
> Cheers,
> 
>   -- Emilio

Neat! Thanks for doing this. It should allow for broader use of Result in media 
code.

Are there any footguns to watch out for when moving into the Result? Looking at 
the changes it appears that if my return type is Result then I can 
just return a BigStruct? I.e. I don't need to do explicitly invoke anything 
else to avoid unintended copies.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

PSA: mozilla::Result is about to understand move semantics

[Emilio Cobos Álvarez]

If you don't use mozilla::Result, then you can stop reading now.

Result always returned the ok / errors by value. That's fine when the ok 
or error values are cheap / small, like integers, but over time we've 
been using Result> / Result / Result, etc.


Furthermore Result never worked with move-only types, so you couldn't 
use Result> or such.


In bug 1418624 I'm about to change mozilla::Result so that:

 * unwrap() / unwrapErr() / unwrapOr() / map() / andThen() move rather 
than copy the result.


 * inspect() and inspectErr() return a reference to the ok / error 
values respectively.


The first change allows the usage of move-only types, and is closer to 
the Rust semantics (though C++ doesn't have the borrow-checker to 
prevent use after moves).


The second change allows having big Result<>s on the stack, while not 
paying extra stack space to inspect them.


I audited and changed all callers that were calling the now-moving 
methods multiple times on the same error to use inspect() / 
inspectErr(). Most of them shouldn't matter, as moving trivial types 
doesn't do anything, but for some cases like some usages of std::wstring 
it should have a positive impact on performance.


Let me know if there's any concerns with this or what not.

Cheers,

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

Re: Documentation for IPDL needs some edit

[Andrew McCreight]

On Sat, Aug 10, 2019 at 7:15 AM  wrote:

> I have started to learn about IPDL and am reading IPDL Tutorial (
> https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Tutorial). It looks
> like the document is old and needs some edits, but this is an area I'm
> still learning so I want to make sure:
>

Yeah, unfortunately most of this documentation was written around 2010, and
it hasn't been updated to keep up with the changes that have happened in
the last 9 years. Probably the most direct way to see what the current
syntax is is to look at the IPDL parser code:
https://searchfox.org/mozilla-central/source/ipc/ipdl/ipdl/parser.py
You can look at the commit history of that file to find the bug numbers for
when things were removed and maybe get some context for that.


> 1. Nearly all of the examples lack explicit modifier before message names.
> AFAIK messages are required to have `async` or `sync` before them since Bug
> 1240871, am I right?
>
Yes, that is right.

>
> 2. The document says `rpc` shouldn't be used for general purpose but still
> the example in "Subprotocols and Protocol Management" section uses it for
> no explicit reason. I think it should use async instead as suggested, what
> do you think?
>

As you found in your followup message, this was replaced with some kind of
priority stuff. I don't remember the specifics.


> 3. `rpc` is mainly for NPAPI per the document, but NPAPI is dead. Why do
> we still need it?
>

NPAPI isn't dead quite yet. It is still used for Flash, which is going to
be supported for at least a few more years, if I remember correctly.


> Not sure this is the right place to discuss about documentations, if I
> posted to a wrong one please inform me.
> ___
> 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: Documentation for IPDL needs some edit

[saschanaz7]

On Monday, August 12, 2019 at 6:29:01 AM UTC+9, j.j. wrote:
> Am Samstag, 10. August 2019 16:10:58 UTC+2 schrieb sasch...@gmail.com:
> > I have started to learn about IPDL and am reading IPDL Tutorial 
> > (https://developer.mozilla.org/en-US/docs/Mozilla/IPDL/Tutorial). It looks 
> > like the document is old and needs some edits, but this is an area I'm 
> > still learning so I want to make sure:
> > 
> > 1. Nearly all of the examples lack explicit modifier before message names. 
> > AFAIK messages are required to have `async` or `sync` before them since Bug 
> > 1240871, am I right?
> > 
> > 2. The document says `rpc` shouldn't be used for general purpose but still 
> > the example in "Subprotocols and Protocol Management" section uses it for 
> > no explicit reason. I think it should use async instead as suggested, what 
> > do you think?
> > 
> > 3. `rpc` is mainly for NPAPI per the document, but NPAPI is dead. Why do we 
> > still need it?
> > 
> > Not sure this is the right place to discuss about documentations, if I 
> > posted to a wrong one please inform me.
> 
> I can't answer any of your questions, but may be this is helpful in case the 
> docs are out of date:
> https://developer.mozilla.org/de/docs/MDN/Feedback
> where you can find this link to report a problem:
> https://github.com/mdn/sprints/issues/new?template=issue-template.md=mdn/sprints/2=user-report

Thanks, I filed https://github.com/mdn/sprints/issues/2006.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

RE: Intent to Ship: Move Extended Validation Information out of the URL bar

[Marissa (Reese) Wood]

I quite like this.  Thank you for the update!

 

Marissa (Reese) Wood, PMP, CISSP  | Cell Phone   303-506-3282 
|   re...@mozilla.com | Slack: #Marissa (Reese)

 

From: firefox-dev  On Behalf Of Johann Hofmann
Sent: Monday, August 12, 2019 10:05 AM
To: Firefox Dev 
Cc: dev-platform ; Wayne Thayer 

Subject: Intent to Ship: Move Extended Validation Information out of the URL bar

 

In desktop Firefox 70, we intend to remove Extended Validation (EV) indicators 
from the identity block (the left hand side of the URL bar which is used to 
display security / privacy information). We will add additional EV information 
to the identity panel instead, effectively reducing the exposure of EV 
information to users while keeping it easily accessible.

 

Before:

 

  

 

 

After:

 

  

 

 

The effectiveness of EV has been called into question numerous times over the 
last few years, there are serious doubts whether users notice the absence of 
positive security indicators and proof of concepts have been  
 pitting EV against domains 
for phishing.

 

More recently, it has been   shown that EV certificates 
with colliding entity names can be generated by choosing a different 
jurisdiction. 18 months have passed since then and no changes that address this 
problem have been identified.

 

The Chrome team recently removed EV indicators from the URL bar in Canary and 
announced  

 their intent to ship this change in Chrome 77. Safari is also no longer 
showing the EV entity name instead of the domain name in their URL bar, 
distinguishing EV only by the green color. Edge is also no longer showing the 
EV entity name in their URL bar.

 

On our side a pref for this (security.identityblock.show_extended_validation) 
was added in   bug 
1572389 (thanks :evilpie for working on it!). We're planning to flip this pref 
to false in   bug 1572936.

 

Please let us know if you have any questions or concerns,

 

Wayne & Johann

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

[desktop] Bugs logged by Desktop Release QA in the last 7 days

[Mihai Boldan]

Hello,

Here's the list of new issues found and filed by the Desktop Release QA 
team in the last 7 days.
Additional details on the team's priorities last week, as well as the 
plans for the current week are available at: https://tinyurl.com/y2wmf9er.

Bugs logged by Desktop Release QA in the last 7 days:

Firefox: Distributions
ASSIGNED - https://bugzil.la/1571639 - distribution.ini bookmarks are 
duplicated during profile refresh


Firefox: Site Identity and Permission Panels
* RESOLVED WONTFIX - https://bugzil.la/1571730 - Autoplay drop down is 
shifted if opened before the content blocking information is loaded
* RESOLVED FIXED - https://bugzil.la/1572016 - Autoplay icon under 
Permissions should be crossed off when 'Block Audio and Video' is selected
* NEW - https://bugzil.la/1572035 - [macOS] Preferences page is opened 
with empty autoplay drop down panel when navigating with Tab+Enter


Firefox: Theme
NEW - https://bugzil.la/1571701 - Tooltip text color for page URL in the 
Site Identity not readable when dragged over tabs bar


Core: DOM: Core & HTML
NEW - https://bugzil.la/1571990 - Unload event test failure - navigation 
timing followup


Core: Localization
RESOLVED FIXED - https://bugzil.la/1572370 - Missing characters in About 
Firefox window on hy-AM locale


Core: User events and focus handling
NEW - https://bugzil.la/1572399 - [Ubuntu] Click on a button an 
additional dotted vertical line appears


DevTools: Inspector: Rules
NEW - https://bugzil.la/1571649 - Inactive CSS - display warnings when 
'display:none' is used for the other rules


This is available as a Bugzilla bug list as well: 
https://tinyurl.com/yyt27aau.


Regards,
Mihai Boldan













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

Intent to Ship: Move Extended Validation Information out of the URL bar

[Johann Hofmann]

In desktop Firefox 70, we intend to remove Extended Validation (EV)
indicators from the identity block (the left hand side of the URL bar which
is used to display security / privacy information). We will add additional
EV information to the identity panel instead, effectively reducing the
exposure of EV information to users while keeping it easily accessible.

Before:


After:


The effectiveness of EV has been called into question numerous times over
the last few years, there are serious doubts whether users notice the
absence of positive security indicators and proof of concepts have been pitting
EV against domains  for
phishing.

More recently, it has been shown  that EV
certificates with colliding entity names can be generated by choosing a
different jurisdiction. 18 months have passed since then and no changes
that address this problem have been identified.

The Chrome team recently removed EV indicators from the URL bar in Canary
and announced their intent to ship this change in Chrome 77
.
Safari is also no longer showing the EV entity name instead of the domain
name in their URL bar, distinguishing EV only by the green color. Edge is
also no longer showing the EV entity name in their URL bar.



On our side a pref for this
(security.identityblock.show_extended_validation) was added in bug 1572389
 (thanks :evilpie for
working on it!). We're planning to flip this pref to false in bug 1572936
.

Please let us know if you have any questions or concerns,

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

Re: Non-header-only headers shared between SpiderMonkey and the rest of Gecko

[Henri Sivonen]

On Tue, Aug 6, 2019 at 8:54 PM Kris Maglione  wrote:
>
> On Tue, Aug 06, 2019 at 10:56:55AM +0300, Henri Sivonen wrote:
> > Do we have some #ifdef for excluding parts of mfbt/ when mfbt/ is being used
> > in a non-SpiderMonkey/Gecko context?
>
> #ifdef MOZ_HAS_MOZGLUE

Thanks. This appears to be undefined for gtests that call in to libxul
code. (Is that intentional?) It appears that MOZILLA_INTERNAL_API is
needed for covering gtests that call into libxul code.

As far as I can tell, after
https://bugzilla.mozilla.org/show_bug.cgi?id=1572364 , ".cpp that
links with jsrust_shared" is detected by:
#if defined(MOZILLA_INTERNAL_API) || \
(defined(MOZ_HAS_MOZGLUE) && defined(ENABLE_WASM_CRANELIFT))

Does that look right? I verified this experimentally by checking that
the above evaluates to false when compiling
--enable-application=tools/update-packaging or
--enable-application=memory.

(Despite advice to the contrary, I still think it's important for
discoverability to put my code in mfbt/TextUtils.h and having it
disabled in contexts that don't link with jsrust_shared. It would be
bad if e.g. mozilla::IsAscii taking a single char and mozilla::IsAscii
taking mozilla::Span weren't discoverable in the same
place. Or even worse if mozilla::IsAscii taking '\0'-terminated const
char* and mozilla::IsAscii taking mozilla::Span weren't
discoverable in the same place.)


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