Re: PSA: new helper class for MozPromise in DOM code

[Ben Kelly]

On Fri, Apr 27, 2018, 3:13 AM Jean-Yves Avenard 
wrote:

> > The class is called DOMMozPromiseRequestHolder.  Here is an example using
> > it:
>
> is that just a refcounted version of MozPromiseRequestHolder?
>

No.  It binds to the global and calls DisconnectIfExists() when the global
invokes DisconnectFromOwner() on it.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

PSA: new CopyableErrorResult class

[Ben Kelly]

Hi all,

I just pushed another helper class that I thought others might find useful.

CopyableErrorResult is a specialized form of ErrorResult.  Its intended to
allow slightly more rich error values to be passed through things like ipdl
structure and MozPromise.  This is useful when implementing web exposed
features that need to execute code in the parent process and surface useful
TypeError messages back to the page.  A simple nsresult loses the developer
friendly messages.  If you are building something like this, then you might
want to consider CopyableErrorResult.

If in doubt, though, please use ErrorResult itself.  It has stricter
checking and should be preferred.  Particularly if there is any chance the
error might contain a js exception.

The CopyableErrorResult has a few properties:

1. Asserts if you try to store a js exception.  It may only contain simple
errors or errors with messages.  In release builds it converts the js
exception to an NS_ERROR_FAILURE.
2. Has a copy constructor, assignment operator, etc.
3. May be safely transferred across threads.
4. Automatically suppresses errors instead of asserting if its not consumed.

These are significant because they mean you can create an ipdl structure or
union type that contains a CopyableErrorResult.  This is not possible with
an ordinary ErrorResult because there is no copy constructor, etc.

It is safer to use CopyableErrorResult in a MozPromise reaction because its
thread safe and does not assert if its destroyed before consumed.  These
are important because MozPromise reactions can cross thread boundaries and
a disconnected promise may not consume its reaction value, etc.

Using CopyableErrorResult allows you to provide error messages along with
TypeError, etc.  Passing just an nsresult provides a worse developer
experience for webdevs.

One quirk you may run into is that CopyableErrorResult may not be used as a
value argument to a method.  This is because it contains a js MOZ_NON_PARAM
value in a union type even through it does runtime checking to avoid ever
actually using that type.  To work around this you do this:

  DoFoo(const CopyableErrorResult& aRv) {
ConsumeResult(CopyableErrorResult(aRv));
  }

This is often needed in MozPromise reaction handlers.

Please let me know if you run into any problems.

Thanks.

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

PSA: new helper class for MozPromise in DOM code

[Ben Kelly]

Hi all,

I pushed a new helper class to inbound today to make it easier to work with
MozPromise in DOM code.  Specifically, it allows you to auto-disconnect a
Thenable request when the global dies.

The class is called DOMMozPromiseRequestHolder.  Here is an example using
it:

#include "mozilla/dom/DOMMozPromiseRequestHolder.h"

// some method...

nsIGlobalObject* aGlobal = GetParentObject();
NS_ENSURE_TRUE(aGlobal, NS_ERROR_DOM_INVALID_STATE_ERR);

RefPtr> holder =
  new DOMMozPromiseRequestHolder(global);

SomeAsyncWork()->Then(
  global->EventTargetFor(TaskCategory::Other), __func__,
  [holder] {
holder->Complete();
// do resolve stuff
  }, [holder] {
holder->Compelte();
// do rejection stuff
  })->Track(*holder);

This code supports multiple overlapping async operations at once.  It also
works on both window and worker globals.

Disconnecting on workers is particularly important since the worker
shutdown may prevent the promise from dispatching a runnable.  This can in
turn trigger assertions in the Thenable's destructor.  The helper class
correctly disconnects the Thenable during worker shutdown without any
additional runnable dispatch.

This is landing in:

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

Please let me know if you run into any problems with it.

Thanks.

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

Re: testing event targets for memory leaks

[Ben Kelly]

PSA, I am landing additional leak tests for

  AudioContext
  IDB
  WebSocket

As part of:

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

On Thu, Apr 5, 2018 at 12:18 PM, Ben Kelly  wrote:

> Hi all,
>
> I recently landed some test infrastructure for testing event targets for
> memory leaks.  This was part of fixing my service worker memory leak in bug
> 1447871.  I wanted to let people know this existed and also ask for help
> writing tests for more event targets.
>
> To repeat, I need help writing more tests.  Please see the list of
> untested APIs at the bottom of this email.
>
> To write an event target leak test you need to import this script:
>
>   
>
> And then call this function:
>
>   await checkForEventListenerLeaks("Worker", useWorker);
>
> The string is just a name to make reporting assertions easier.  The second
> argument is a callback function where you exercise the API you want to test
> for leaks.  In this case I'm testing the worker API:
>
>   async function useWorker(contentWindow) {
> contentWindow.messageCount = 0;
> let w = new contentWindow.Worker("data:application/javascript,self.
> postMessage({})");
> w.onerror = _ => {
>   contentWindow.errorCount += 1;
> };
> await new Promise(resolve => {
>   w.onmessage = e => {
> contentWindow.messageCount += 1;
> resolve();
>   };
>});
> is(contentWindow.messageCount, 1, "message should be received");
>   }
>
> The callback function should add an event listener that holds the given
> contentWindow alive.  Then try to leave the API in an active state to see
> if its properly torn down when the test harness closes the window.
>
> This particular test found that the Worker API leaks.  See bug 1451381.
>
> I've written tests for a number of APIs so far:  ServiceWorker*, XHR,
> Animation, MediaQueryList, EventSource, BroadcastChannel, MessageChannel,
> SharedWorker, AbortSignal, WebSocket, Notification
>
> The WebSocket and Notification tests have not landed yet because they
> trigger assertions.  These look like bugs in the respective APIs.
>
> There are many, many event targets in the system, though.  Using searchfox
> to look for subclasses of DOMEventTargetHelper I came up with a list of
> event targets that are not currently tested.  Some of these are deprecated
> or not exposed.  But a lot of these are web exposed.  It would be really
> great to get this kind of simple leak test written for these APIs.
>
> ContentFrameMessageManager
> DOMRequest
> ScreenOrientation
> BatteryManager
> OffscreenCanvas
> ConstructibleEventTarget
> FetchObserver
> FileReader
> IDBFileHandle
> IDBMutableFileHandle
> IDWrapperCache (and sub classes)
> DOMMediaStream
> MediaDevices
> MediaRecorder
> MediaStreamTrack
> MediaTrack
> MediaTrackList
> TextTrack
> TextTrackCue
> TextTrackList
> MediaKeySession
> ImageCapture
> MediaSource
> SourceBuffer
> SourceBufferList
> AudioContext
> AudioNode
> SpeechRecognition
> SpeechSynthesis
> SpeechSynthesisUtterance
> MIDIAccess
> MIDIPort
> Connection
> TCPServerSocket
> TCPSocket
> UDPSocket
> PaymentRequest
> Performance
> PermissionStatus
> PresentationAvailability
> PresentationConnection
> PresentationConnectList
> PresentationRequest
> VRDisplay
> FontFaceSet
> ChannelWrapper
> StreamFilter
>
> If one of these targets falls in your area, please try to find the time to
> write a small test as described above.  Also, please link it against the
> meta bug here:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1451787
>
> Thanks!
>
> Ben
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: testing event targets for memory leaks

[Ben Kelly]

On Mon, Apr 9, 2018 at 3:16 PM, Randell Jesup  wrote:

> I'm surprises that DOMDataChannel wasn't found:
> nsDOMDataChannel.h:
>   class nsDOMDataChannel final : public mozilla::DOMEventTargetHelper,
>
> perhaps you were looking for "public DOMEventTargetHelper"?
>

Yep.  That was indeed my lame search.


>
> I also find nsScreen and nsDOMOfflineResourceList using
> mozilla::DOMEventTargetHelper
>
> Are there any events implemented in JS that we need to worry about?  For
> example, there are a lot of events (and a number of objects) defined for
> WebRTC as part of dom/media/PeerConnection.js
>

Having test coverage would be prudent, I think.  I'm not sure what can
break in the js event target world.

Thanks.

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

Re: testing event targets for memory leaks

[Ben Kelly]

On Mon, Apr 9, 2018 at 12:32 PM, Ben Kelly  wrote:

> On Mon, Apr 9, 2018 at 12:06 PM, Boris Zbarsky  wrote:
>
>> On 4/5/18 1:11 PM, Ben Kelly wrote:
>>
>>> 1. Make sure you set the nsIGlobalObject owner by passing it to the DETH
>>> constructor or by calling BindToOwner().
>>>
>>
>> Can we just enforce that by:
>>
>> 1)  Removing the no-arg constructor from DETH.
>> 2)  Making the arg-taking constructors MOZ_ASSERT that the arg is not
>> null.
>>
>> ?  We'd need to fix some existing code, but after that should hopefully
>> be in a better place...
>>
>
> This would be nice to do yes, but was out of scope for the time I had.  I
> also suspect, though, that the non-binding DETH constructor is used for
> objects created using the separate Init() method pattern.  Not sure if its
> fair to try to convert all of those.
>
> Also keep in mind that binding to the global on a worker thread is
> somewhat new.  Many DETH objects only try to bind nsPIDOMWindowInner, but
> it would be better to always bind nsIGlobalObject now.
>
> Anyway, we should probably file a bug to consider it.
>

Filed as:

https://bugzilla.mozilla.org/show_bug.cgi?id=1452705
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: testing event targets for memory leaks

[Ben Kelly]

On Mon, Apr 9, 2018 at 12:06 PM, Boris Zbarsky  wrote:

> On 4/5/18 1:11 PM, Ben Kelly wrote:
>
>> 1. Make sure you set the nsIGlobalObject owner by passing it to the DETH
>> constructor or by calling BindToOwner().
>>
>
> Can we just enforce that by:
>
> 1)  Removing the no-arg constructor from DETH.
> 2)  Making the arg-taking constructors MOZ_ASSERT that the arg is not null.
>
> ?  We'd need to fix some existing code, but after that should hopefully be
> in a better place...
>

This would be nice to do yes, but was out of scope for the time I had.  I
also suspect, though, that the non-binding DETH constructor is used for
objects created using the separate Init() method pattern.  Not sure if its
fair to try to convert all of those.

Also keep in mind that binding to the global on a worker thread is somewhat
new.  Many DETH objects only try to bind nsPIDOMWindowInner, but it would
be better to always bind nsIGlobalObject now.

Anyway, we should probably file a bug to consider it.

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

Re: testing event targets for memory leaks

[Ben Kelly]

On Thu, Apr 5, 2018 at 12:18 PM, Ben Kelly  wrote:

> If one of these targets falls in your area, please try to find the time to
> write a small test as described above.  Also, please link it against the
> meta bug here:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1451787
>

Sorry to self-reply to an already long email, but...

Here are some hints to fix leaks in DOMEventTargetHelper (DETH) sub-classes:

1. Make sure you set the nsIGlobalObject owner by passing it to the DETH
constructor or by calling BindToOwner().
2. Override the DisconnectFromOwner() method to perform any cleanup.  This
is now consistently called when the window is doomed.

Failing to do (1) caused MediaQueryList to leak (bug 1450271).  We believe
failing to do (2) is causing the worker API to leak.

In the past code has had to use nsIObserver to listen for
inner-window-destroyed in order to do cleanup.  This was due to
DisconnectFromOwner() not always getting called consistently.  This should
now be fixed as of bug 1450266.  It should now be possible to replace these
old listeners with a simpler DisconnectFromOwner() override.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

testing event targets for memory leaks

[Ben Kelly]

Hi all,

I recently landed some test infrastructure for testing event targets for
memory leaks.  This was part of fixing my service worker memory leak in bug
1447871.  I wanted to let people know this existed and also ask for help
writing tests for more event targets.

To repeat, I need help writing more tests.  Please see the list of untested
APIs at the bottom of this email.

To write an event target leak test you need to import this script:

  

And then call this function:

  await checkForEventListenerLeaks("Worker", useWorker);

The string is just a name to make reporting assertions easier.  The second
argument is a callback function where you exercise the API you want to test
for leaks.  In this case I'm testing the worker API:

  async function useWorker(contentWindow) {
contentWindow.messageCount = 0;
let w = new
contentWindow.Worker("data:application/javascript,self.postMessage({})");
w.onerror = _ => {
  contentWindow.errorCount += 1;
};
await new Promise(resolve => {
  w.onmessage = e => {
contentWindow.messageCount += 1;
resolve();
  };
   });
is(contentWindow.messageCount, 1, "message should be received");
  }

The callback function should add an event listener that holds the given
contentWindow alive.  Then try to leave the API in an active state to see
if its properly torn down when the test harness closes the window.

This particular test found that the Worker API leaks.  See bug 1451381.

I've written tests for a number of APIs so far:  ServiceWorker*, XHR,
Animation, MediaQueryList, EventSource, BroadcastChannel, MessageChannel,
SharedWorker, AbortSignal, WebSocket, Notification

The WebSocket and Notification tests have not landed yet because they
trigger assertions.  These look like bugs in the respective APIs.

There are many, many event targets in the system, though.  Using searchfox
to look for subclasses of DOMEventTargetHelper I came up with a list of
event targets that are not currently tested.  Some of these are deprecated
or not exposed.  But a lot of these are web exposed.  It would be really
great to get this kind of simple leak test written for these APIs.

ContentFrameMessageManager
DOMRequest
ScreenOrientation
BatteryManager
OffscreenCanvas
ConstructibleEventTarget
FetchObserver
FileReader
IDBFileHandle
IDBMutableFileHandle
IDWrapperCache (and sub classes)
DOMMediaStream
MediaDevices
MediaRecorder
MediaStreamTrack
MediaTrack
MediaTrackList
TextTrack
TextTrackCue
TextTrackList
MediaKeySession
ImageCapture
MediaSource
SourceBuffer
SourceBufferList
AudioContext
AudioNode
SpeechRecognition
SpeechSynthesis
SpeechSynthesisUtterance
MIDIAccess
MIDIPort
Connection
TCPServerSocket
TCPSocket
UDPSocket
PaymentRequest
Performance
PermissionStatus
PresentationAvailability
PresentationConnection
PresentationConnectList
PresentationRequest
VRDisplay
FontFaceSet
ChannelWrapper
StreamFilter

If one of these targets falls in your area, please try to find the time to
write a small test as described above.  Also, please link it against the
meta bug here:

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

Thanks!

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

Re: Phabricator and Bugzilla

[Ben Kelly]

On Sat, Mar 31, 2018, 10:09 AM Mark Côté  wrote:

> Regarding comment and flag mirroring, we've discussed this before:
>
>
> https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/e3Pi-_FpBgAJ
>
> https://groups.google.com/d/msg/mozilla.dev.platform/Y8kInYxo8UU/tsF7UfxvBgAJ
>
> Given that Phabricator is still new, I don't see any reason to reopen that
> discussion at this point, aside from noting that we have work in progress
> to include Phabricator requests in BMO's My Dashboard and notifications
> indicator (https://bugzilla.mozilla.org/show_bug.cgi?id=1440828).
>

What about comment mirroring?  On my mobile so I haven't read all the past
threads, but my recollection is that your team did not want to implement
that feature.  Personally, this is a huge concern for me.

Thanks.

Ben


> As for interdiffs, feel free to file a bug with any problems you see.  We
> have a good relationship with upstream and can pass those on.  Similarly
> with method names (which has been mentioned before but I can't find where
> at the moment).
>
> There is official documentation at
> https://secure.phabricator.com/book/phabricator/ which is linked from our
> Mozilla-specific docs (
> http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html) which
> in turn is linked in the left-hand menu in Phabricator.  We can expand our
> own docs as needed if there are areas that are particularly confusing due
> to, say, expectations carried over from our other code-review tools.
>
> Mark
>
> On Thursday, 29 March 2018 13:45:28 UTC-4, smaug  wrote:
> > Hi all,
> >
> > just some random notes about Phabricator.
> >
> > I've been reviewing now a bunch of patches in Phabricator and the
> initial feeling from
> > reviewer's point of view is that it is ok. Not great, but ok.
> > MozReview's interdiff, when it works, is easier to use or at least to
> discover than Phabricator's.
> > MozReview does also show the method name in which the code lives. This
> latter thing is actually a big
> > + to MozReview. (Same works in Bugzilla too when reviewing raw -P
> patches at least. No idea about splinter, since I never use it).
> > If Pabricator has the feature, I haven't found it yet - its UI is a tad
> confusing occasionally.
> >
> > What is currently rather broken is the flag synchronization between
> bugzilla and Phabricator.  One doesn't see in Bugzilla whether review has
> been
> > asked or whether review has been denied (r-). I believe people asking
> reviews from me set the r? flag manually in bugzilla.
> > Having an easy way to see that r- has been given to a patch is very
> valuable to the reviewer when looking at the bug again.
> > Oddly enough, approving the patch in Phabricator does set r+ in Bugzilla.
> >
> > Not mirroring comments from Phabricator to Bugzilla has already made
> following reasoning for certain code changes harder.
> > It is so easy to include non-code level information in review comments.
> So far I haven't had a case where I would have needed to look at the blame
> and
> > try to find relevant information both in bugzilla and in phabricator,
> but that will obviously happen if we don't do the mirroring and it will slow
> > down reviewing in the future (since looking at the history/reasoning for
> the code changes is a big part of code reviewing).
> >
> > I'm sure I haven't found all the tricks Phabricator has to help
> reviewing. Is there some reviewing-in-Phabricator-for-dummies doc?
> >
> >
> > I haven't asked reviews in Phabricator (nor in MozReview), so can't
> comment on how they compare from patch author's point of view.
> >
> >
> >
> > br,
> >
> >
> >
> > -Olli
>
> ___
> 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: CPU core count game!

[Ben Kelly]

That page says "physical cores", so its not taking into account hyper
threading, right?  So even a high end macbook pro falls in that category?

On Tue, Mar 27, 2018 at 5:02 PM, Mike Conley  wrote:

> Thanks for drawing attention to this, sfink.
>
> This is likely to become more important as we continue to scale up our
> parallelization with content processes and threads.
>
> On 21 March 2018 at 14:54, Steve Fink  wrote:
>
> > Just to drive home a point, let's play a game.
> >
> > First, guesstimate what percentage of our users have systems with 2 or
> > fewer cores.
> >
> > Then visit https://hardware.metrics.mozilla.com/#goto-cpu-and-memory to
> > check your guess.
> >
> > (I didn't say it was a *fun* game.)
> >
> >
> > ___
> > 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: PSA: nsIURI implementations are now threadsafe

[Ben Kelly]

On Mon, Mar 26, 2018 at 10:47 AM, Valentin Gosu 
wrote:

> Yes, that is definitely something we want to fix, but not very
> straightforward. We have quite a few URI implementations, and a bunch more
> protocol handlers.
>
> https://github.com/valenting/gecko/wiki/Threadsafe-URIs-
> progress#protocol-handler-implementations
>

I wonder if it would be worth adding something like:

NS_ThreadsafeNewURI()

That returns an error code if it detects that the URL does not match any of
the threadsafe URL implementations.  This would allow code to try
threadsafe parsing first and only fall back to a main thread bounce if its
an oddball URL.  Right now we hardcode a bunch of http/https/nsStandardURL
things in various places to accomplish this.


>
>
> On 26 March 2018 at 15:24, Ben Kelly  wrote:
>
>> Do we have any plan to be able to use NS_NewURI() off-main-thread?  I
>> thought that was included here, but I see now that it is not.  The initial
>> URL parse OMT is important for truly being able to remove all our "bounce
>> to the main thread for URL stuff" legacy code.
>>
>> On Fri, Mar 23, 2018 at 8:25 AM, Valentin Gosu 
>> wrote:
>>
>>> Hello everyone,
>>>
>>> I would like to announce that with the landing of bug 1447194, all nsIURI
>>> implementations in Gecko are now threadsafe, as well as immutable. As a
>>> consequence, you no longer have to clone a URI when you pass it around,
>>> as
>>> it's guaranteed not to change, and now it's OK to release them off the
>>> main
>>> thread.
>>>
>>> If you need to change a nsIURI, you should use the nsIURIMutator
>>> interface
>>> (in JavaScript - just call .mutate() on the URI) or the NS_MutateURI
>>> <https://searchfox.org/mozilla-central/source/netwerk/test/g
>>> test/TestURIMutator.cpp#22>
>>> helper class (in C++).
>>>
>>> More info here:
>>> https://wiki.mozilla.org/Necko/nsIURI
>>>
>>> If you find any bugs, make them block bug 922464 (OMT-nsIURI)
>>>
>>> I'd like to thank everyone who helped review the patches, especially
>>> Honza
>>> Bambas who reviewed most of my patches.
>>>
>>> Cheers!
>>> ___
>>> 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: nsIURI implementations are now threadsafe

[Ben Kelly]

This is so great.  Thank you!

One question that comes to mind, though, is there any chance this could be
uplifted to 60?  As we start doing more OMT nsIURI stuff its going to
become difficult to uplift code to 60ESR.

On Fri, Mar 23, 2018 at 8:25 AM, Valentin Gosu 
wrote:

> Hello everyone,
>
> I would like to announce that with the landing of bug 1447194, all nsIURI
> implementations in Gecko are now threadsafe, as well as immutable. As a
> consequence, you no longer have to clone a URI when you pass it around, as
> it's guaranteed not to change, and now it's OK to release them off the main
> thread.
>
> If you need to change a nsIURI, you should use the nsIURIMutator interface
> (in JavaScript - just call .mutate() on the URI) or the NS_MutateURI
>  TestURIMutator.cpp#22>
> helper class (in C++).
>
> More info here:
> https://wiki.mozilla.org/Necko/nsIURI
>
> If you find any bugs, make them block bug 922464 (OMT-nsIURI)
>
> I'd like to thank everyone who helped review the patches, especially Honza
> Bambas who reviewed most of my patches.
>
> Cheers!
> ___
> 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: nsIURI implementations are now threadsafe

[Ben Kelly]

Do we have any plan to be able to use NS_NewURI() off-main-thread?  I
thought that was included here, but I see now that it is not.  The initial
URL parse OMT is important for truly being able to remove all our "bounce
to the main thread for URL stuff" legacy code.

On Fri, Mar 23, 2018 at 8:25 AM, Valentin Gosu 
wrote:

> Hello everyone,
>
> I would like to announce that with the landing of bug 1447194, all nsIURI
> implementations in Gecko are now threadsafe, as well as immutable. As a
> consequence, you no longer have to clone a URI when you pass it around, as
> it's guaranteed not to change, and now it's OK to release them off the main
> thread.
>
> If you need to change a nsIURI, you should use the nsIURIMutator interface
> (in JavaScript - just call .mutate() on the URI) or the NS_MutateURI
>  TestURIMutator.cpp#22>
> helper class (in C++).
>
> More info here:
> https://wiki.mozilla.org/Necko/nsIURI
>
> If you find any bugs, make them block bug 922464 (OMT-nsIURI)
>
> I'd like to thank everyone who helped review the patches, especially Honza
> Bambas who reviewed most of my patches.
>
> Cheers!
> ___
> 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: FYI: Short Nightly Shield Study involving DNS over HTTPs (DoH)

[Ben Kelly]

Note, this effort is already being reported in the tech press based on this
thread.  For example:

https://www.theregister.co.uk/AMP/2018/03/20/mozilla_firefox_test_of_privacy_mechanism_prompts_privacy_worries/

A blog post does sound like a good idea.

On Mon, Mar 19, 2018, 11:33 PM Dave Townsend  wrote:

> As one of the folks who brought up the initial concern let me be clear that
> at this point my only real concern here is one of optics. The DoH service
> we're using is likely more private than anything the user is currently
> using. I just don't want to see random folks on the web "discover" these
> DoH requests and not be able to find details about them and so cause a
> press cycle.
>
> Having a good blog post up, particularly if it jumps out when you search
> for the address that we're querying for DoH and gives good instructions on
> how to opt-out does a lot to mitigate that. Reducing the population would
> likely also help with that if that is an option.
> ___
> 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: Intent to implement and ship: spec compliance Promise microtask behavior

[Ben Kelly]

I just want to give a huge thank you to everyone who worked on this.  Its
such a monumental task to change such a core feature, especially with new
tests that might depend on the old behavior being added all the time.

Thank you!

Ben

On Wed, Feb 28, 2018 at 6:58 AM, Hiroyuki Ikezoe 
wrote:

> Summary: We are about to land bug 1193394 which will change microtask
> behavior that our microtask behavior complied with the HTML spec.
>
> We had fixed all test failures but still it's possible that new failures
> will
> appear before the change gets merged into mozilla-central.  If we found any
> failed tests we will disable it temporarily and file a bug to enable it
> respectively.  If new test you have locally started failing after this
> change,
> the document that Bevis wrote [1] would be helpful to make the test pass
> with
> the new behavior.  Bevis' original message [2] has some background of
> this, so
> it would be also helpful to understand this change.
>
> Big thanks to Olli who started the initial work and Bevis who took over
> his work
> and :arai who had been fixing tests that failed by the microtask change in
> various
> areas.  And big thanks to everyone involved!
>
> Bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1193394
> Standard: https://html.spec.whatwg.org/multipage/webappapis.html#enque
> uejob(queuename,-job,-arguments)
> Platform coverage: All
> Target release: Firefox 60
> Preference: None. We tried to add a pref but didn't add since it will make
>  things more complicated and hard to maintain.
>  See https://bugzilla.mozilla.org/show_bug.cgi?id=1420816#c9 for the
> detail.
> DevTools bug: None
> web-platform-tests: There are test cases that this change makes the tests
> pass.
> Do other browser engines implement this?: Chrome and Safari as per the
> result of
>  a wpt [3].
>
> [1] https://docs.google.com/presentation/d/1momsC3suU8m-CrdZyYD_
> 6QATATehjzZHbkGmL6KsmSk/edit#slide=id.g28ecd2197a_0_269
> [2] https://groups.google.com/forum/#!msg/mozilla.dev.platform/
> naB6gaevSSo/w-29kbpUBQAJ
> [3] https://wpt.fyi/html/webappapis/scripting/event-loops/task_
> microtask_ordering.html
>
> ___
> 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: C++ virtual function declarations should specify only one of virtual, final, or override

[Ben Kelly]

Are we supposed to just use override or final on methods that are overriden
when the class itself is marked final?

Personally writing final again seems redundant with the class level final
and the override keyword seems more informative.

On Fri, Feb 16, 2018 at 3:36 PM, Chris Peterson 
wrote:

> Mozilla's C++ style guide [1] says (since 2015) virtual function
> declarations should specify only one of `virtual`, `final`, or `override`.
>
> Over the weekend, I will land a mach lint check (bug 1436263) that will
> warn about some virtual style violations such as:
>
>   virtual void Bad1() final
>   void Bad2() final override
>   void Bad3() override final
>
> It won't warn about the redundant `override` in `virtual void NotBad()
> override` at this time because there are 8000+ instances in
> mozilla-central. Also, virtual/override is more of a style issue whereas
> virtual/final can mask real bugs.
>
> A clang-tidy check would be more thorough, but this mach lint is better
> than nothing until someone steps up to write a proper clang plugin. :) We
> had a clang-tidy check but it was disabled recently (bug 1420366) because
> it was too noisy (because it analyzed the post-processed source after
> NS_IMETHOD macros had been expanded to `virtual`).
>
>
> [1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_g
> uide/Coding_Style
> ___
> 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

PSA: service worker code has moved

[Ben Kelly]

Hi all,

Just FYI, we have moved the service worker code and tests from:

  dom/workers
  dom/workers/test/serviceworkers

To:

  dom/serviceworkers
  dom/serviceworkers/test

We did this because the service worker feature is about much more than just
spawning a thread in the current process.  Its really a cross-process
feature with hooks throughout the browser.  It was growing in size and
cluttering things up in dom/workers.

This change is now in mozilla-inbound now.  It was landed in:

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

Sorry for any harshed patches or confusion over intermittent bug title
matching.

Let me know if there are problems.  Thanks.

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

Re: Intent to Unship: Application Cache over Insecure Contexts

[Ben Kelly]

On Fri, Jan 19, 2018 at 12:26 PM, Mike Taylor  wrote:

> > When the pref is set to false the API will be removed:
> >
> >   -
> >
> >   window.applicationCache will be removed
> >   -
> >
> >   The cache service Firefox implements for AppCache will be disabled over
> >   Insecure Contexts
> >
> >
> > When the pref is set to true the code will produce an additional
> developer
> > console warning about the removal timeline.
> >
> > In Nightly and Early beta for 60; the pref will be set to false removing
> > the API.
>
> It will be interesting to see if we get reports of pages (that don’t
> feature test) throwing with the missing applicationCache global. A few
> years (and laptops) ago I had done some site corpus grepping — I’ll see if
> I can find any of that data.
>

Its been suggested before that we could leave the applicationCache global
in place, but just make it do nothing in insecure contexts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Requiring secure contexts for new features

[Ben Kelly]

On Tue, Jan 16, 2018 at 1:11 PM, Anne van Kesteren  wrote:

> * Modules might want to look into ways of enforcing this
> programmatically, to ease ongoing maintenance and guide everyone to do
> the right thing without having to ask/review/etc. E.g.,
> https://bugzilla.mozilla.org/show_bug.cgi?id=1429014 has some ideas
> for enforcement around our bindings.
>

FYI, Boris landed a change that requires the author to mark the test for an
interface as "insecureContext: true" if it should be permitted outside of a
secure context.  Obviously this should only be permitted going forward if
it meets one of our exception criteria.

Thanks.

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

Re: Refactoring proposal for the observer service

[Ben Kelly]

On Thu, Jan 4, 2018 at 4:35 PM, Gabriele Svelto  wrote:

> On 03/01/18 23:30, Ben Kelly wrote:
> > Could we use our existing idl/webidl/ipdl for this?  It would be nice
> > not to have to maintain another code generator in the tree if possible.
>
>
> AFAIK there is no way in IDL to declare an enum. Constants can be
> declared but one needs to manually assign them a value which makes it
> unsuitable for this task. Also there's no way to attach other metadata
> to the declaration (such as a mandatory comment).
>

We can't have a comment explaining "please add any new constants in
sequential order in the following list"?  Or make your "generator" create
the idl which then creates the js/c++?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Refactoring proposal for the observer service

[Ben Kelly]

On Thu, Jan 4, 2018 at 10:00 AM, Nathan Froyd  wrote:

> On Wed, Jan 3, 2018 at 5:30 PM, Ben Kelly  wrote:
> > On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto 
> wrote:
> >> So after validating my approach in that bug (which is almost ready) I've
> >> thought that it might be time to give the observer service the same
> >> treatment. First of all we'd have a list of topics (I've picked YAML for
> >> the list but it could be anything that fits the bill):
> >
> > Could we use our existing idl/webidl/ipdl for this?  It would be nice
> not to
> > have to maintain another code generator in the tree if possible.
>
> I don't understand this objection on two levels:
>

It wasn't an objection.  It was a question.  If possible, can we use
something we already have?  The answer can be "no".


> 1) Why does maintaining another code generator in tree hurt anything?
> We have many one-off code generators for specific purposes:
>
> https://searchfox.org/mozilla-central/search?q=GENERATED_
> FILES&case=false®exp=false&path=moz.build
>
> Some of these use non-Python generators (e.g. the a11y files and gfx
> shaders), but there are probably enough to count on multiple hands.
>
> I would expect modifications of the code generator to be infrequent,
> and the code generator itself is liable to be a screenful or so of
> straightforward code.
>

Sure, but having someone understand and able to modify additional code
generators does require additional effort.  Its not impossible, but it
makes it harder.  So it would be nice to avoid if we can.  If we can't
avoid it, then ok.


> - WebIDL enums (I think), which would carry a large space penalty and
> make everything that wants to use the observer service depend on code
> from dom/, which seems undesirable.
> - IDL enums, which aren't reflected into JS, so we'd need some custom
> work there.
>

Yea, I was mostly thinking webidl or idl enums.  The enums could be stuck
on an idl interface as constants, no?

Anyway, I was mostly just asking that we consider these options before
writing something new.  If they're not a good fit, then so be it.

Thanks.

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

Re: Refactoring proposal for the observer service

[Ben Kelly]

On Wed, Jan 3, 2018 at 5:09 PM, Gabriele Svelto  wrote:

> So after validating my approach in that bug (which is almost ready) I've
> thought that it might be time to give the observer service the same
> treatment. First of all we'd have a list of topics (I've picked YAML for
> the list but it could be anything that fits the bill):
>

Could we use our existing idl/webidl/ipdl for this?  It would be nice not
to have to maintain another code generator in the tree if possible.

Otherwise seems like a good idea.

Thanks.

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

overly strict eslint rules

[Ben Kelly]

Hello,

First let me say, I don't like participating in style discussions on the
list.  I don't think they are productive in general and I don't want to
have one of those threads here.

I feel I need to raise as an issue, though, as eslint rules are being
pushed out into components with what seem like overly restrictive style
rules.

I have no problem with eslint rules that check for language safety issues.
For example, verifying that there are block braces on single line
if-statements has safety value.  Verifying that semi-colons are used has
safety values.  These are things that can break when new code is added to
the file.

But I also see rules about cosmetic things like what kind of quotes must be
used for strings.  For example:

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

AFAICT this kind of rule does not have any tangible safety benefit.  Its
purely a cosmetic style choice.  I don't see why we should bounce patches
out of the tree if the author and reviewer of a component prefer to use
single quotes instead of double quotes in a file.

I also find this kind of cosmetic rule especially frustrating because we do
not run eslint by default.  AFAIK it does not even install with mach
bootstrap.  If we are going to enforce this kind of thing these checks
should be run as part of a build or when executing tests.  They should not
require manual installation of tools and running separate commands.

As a further anecdote, I have also had to fight eslint rules in devtools
which required things like simultaneously using an extreme indent value and
not exceeding a line width limit.  Basically conflicting and impossible
rules to satisfy.  I ended up wasting close to an hour figuring out an ugly
work around for the problem.  It did not feel like the lint was providing
value.

Anyway, I just want to make a plea that we don't push out overly
restrictive eslint rules for cosmetic reasons.  We should be spending our
time building the browser and not trying to do gymnastics to meet some
arbitrary set of cosmetic rules.  Please focus eslint rules on things that
provide code safety value.

Thanks.  Have a great holiday.

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

Re: Next year in web-platform-tests

[Ben Kelly]

I know you aware, but just mentioning for the list:

We need WPT coverage on Android to fully rely on it as our primary test
suite.  Particularly while Android's config deviates so far from desktop.

On Dec 15, 2017 10:40 AM, "James Graham"  wrote:

> Following the summary of what we achieved in wpt in the last year, I'd
> like to solicit input from the gecko community to inform the
> priorities of various pieces of wpt work for the next year.
>
> In order to maximise the compatibility benefit we have two long term
> objectives for the web-platform-tests work at Mozilla:
>
> * Make writing writing web-platform-tests the default for all
>   web-exposed features, so that we only write unshared tests in
>   exceptional cases (e.g. where there is a need to poke at Gecko
>   internals).
>
> * Ensure that gecko developers are using the web-platform-tests
>   results to avoid interoperability issues in the features they
>   develop.
>
> Obviously we are some way from achieving those goals. I have a large
> list of things that I think will make a difference, but obviously I
> have a different perspective to gecko developers, so getting some
> feedback on the priorities that you have would be good (I know I have
> already have conversations with several people, but it seems good to
> open up the question to a broader audience). In particular
> it would help to hear about things that you would consider blockers to
> removing tests from non-wpt suites that are duplicated in wpt
> (assuming exact duplication), and limitations either in the
> capabilities of wpt or in the workflow that lead to you writing other
> test types for cross-browser features.
>
> Thanks
>
> (Note: I set the reply-to for the email version of this message to be
> off-list as an experiment to see if that avoids the anchoring effect where
> early replies set the direction of all subsequent discussion. But I'm very
> happy to have an on-list conversation about anything that you feel merits a
> broader audience).
> ___
> 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: new code to help with cross-process windows/workers (and Clients API)

[Ben Kelly]

On Dec 9, 2017 8:25 AM, "Gijs Kruitbosch"  wrote:

On 08/12/2017 20:23, Ben Kelly wrote:

> Please let me know if you have any question or if you think you have a
> feature that could integrate with the clients infrastructure.  While the
> initial implementation is limited to Clients API needs, I've tried to
> design it to support other internal uses.  I'd be very happy to help get
> new features added to support browser chrome or native code.
>

Sorry if this was answered by implication somewhere in the OP already (I
looked, but must have missed it!), but is this infrastructure
exposed/available to Chrome JS, and message manager messages (as opposed to
C++-based IPC with ipdl etc.) ?


Not yet, but I have designed it to support that.

Probably the first step would be to expose the Clients WebAPI to browser
chrome script.  We could then add more chrome-only features as use cases
arise.

Can you file a bug in Core:DOM and describe a bit more how you would like
to use it?

I just want to make sure anything we expose fits our browser chrome use
cases well.

In particular, in situations where we currently have content scripts that
need to go ask the parent process something, and the parent then needs to
go back to the content process with an answer, without losing the
"reference" to the frame concerned (which may be a subframe, so just
knowing the target browser is not enough), that is currently annoying to
implement (requires e.g. manual window id tracking in the content process).
It sounds like this API might make this type of thing easier to implement
if we could use it from chrome JS.


The Clients API is all promise based and I expect any future API would be
as well.  So hopefully that will make things easier.

Thanks.

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

Re: new code to help with cross-process windows/workers (and Clients API)

[Ben Kelly]

On Fri, Dec 8, 2017 at 4:38 PM, Mike Conley  wrote:

> >> I personally think this would be useful, and will (probably) only
> happen if
> >> you do it now, before you move on to the next thing.  +1 for
> documentation!
>
> I concur.
>

Alright.  I'll work on a blog post next week.  (I'm not going to be in
Austin for the work week.)

Thanks.

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

new code to help with cross-process windows/workers (and Clients API)

[Ben Kelly]

Hi all,

I just want to give you a heads up about some new infrastructure that is
landing in the tree.  In particular, we now have:

1. A central list of windows and workers for the entire browser in the
parent process.  This includes information on their origin, url, and what
process they are running in.  This lives in ClientManagerService.

2. A structure called ClientInfo which lets you refer to a particular
window or worker.  This can be passed between threads/processes and then
used to attach to the running window/worker by creating a ClientHandle
object.

3. ClientHandle provides initial support for focusing and navigating
windows.  You can also postMessage() both window and worker clients.

4. A ClientManager interface allows you to query the list of clients in the
system and open windows regardless of your current process/thread.

In the future we may move values that apply to both windows and workers to
the new ClientInfo type.  For example, we have plans to move CSP from
nsIPrincipal.  Other data like referrer-policy would that applies to both
windows/workers would also be a good fit.

Before adding new values to nsIDocument or WorkerPrivate, please consider
if they would fit better in the clients infrastructure layer.

Over time it would be nice to move towards the client types since it will
allow us to write more code that "just works" with windows and workers.  We
can start removing the conditional "if nsIDocument do X else if
WorkerPrivate do Y" we have in the tree.

This code has been implemented so far in bug 1293277 and its dependents in
order to support a multi-e10s Clients WebAPI:

https://developer.mozilla.org/en-US/docs/Web/API/Clients

Initially we will be using these new features only in Clients API on
service worker threads, but the design supports using it other contexts as
well.  I have a spec issue open to support content windows as well.

If there is a desire to access these features from browser chrome script,
please let me know.  I think this should be possible once we have use
cases.  While not as powerful as our various xpcom components, it might be
more convenient to use for some cases.

Please let me know if you have any question or if you think you have a
feature that could integrate with the clients infrastructure.  While the
initial implementation is limited to Clients API needs, I've tried to
design it to support other internal uses.  I'd be very happy to help get
new features added to support browser chrome or native code.

Thanks!

Ben

P.S. I was also thinking of doing a blog post with diagrams of how it works
if people think that would be useful.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: increased chance of new leaks due to delayed shutdown issue

[Ben Kelly]

Actually this was backed out from m-c for a single failing android test
this morning.  So please be aware we are at risk of introducing leaks again.

On Sat, Dec 2, 2017 at 9:27 PM, Ben Kelly  wrote:

> Both fixes have stuck and are in the latest nightly.  Sorry for the
> headaches here.
>
> On Dec 1, 2017 1:15 PM, "Ben Kelly"  wrote:
>
>> FYI, we have a fix identified for the late shutdown leak as well.  I will
>> push them to inbound once the trees re-open.
>>
>> On Thu, Nov 30, 2017 at 11:18 PM, Ben Kelly  wrote:
>>
>>> Hi all,
>>>
>>> I just wanted to send a note about a bug in nightly which is leading to
>>> delayed shutdowns.  Currently nightly is taking 5 to 10 seconds to shutdown.
>>>
>>> Bisection has shown this was introduced by my landing in:
>>>
>>> https://bugzilla.mozilla.org/show_bug.cgi?id=1419536
>>>
>>> I have a fix here:
>>>
>>> https://bugzilla.mozilla.org/show_bug.cgi?id=1420594
>>>
>>> But I discovered upon pushing the fix that at least one new leak has
>>> crept in while shutdowns have been delayed.  I'm currently bisecting on try
>>> to figure out what happened.  I am, however, worried about more leaks
>>> appearing while I figure this out.
>>>
>>> If you are touching anything that might live until shutdown, please
>>> include the patches from bug 1420594 to avoid the delayed shutdown issue.
>>>
>>> I'll send another mail once the fix lands and sticks.
>>>
>>> Thanks for your help.  Sorry for the trouble.
>>>
>>> Ben
>>>
>>
>>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: increased chance of new leaks due to delayed shutdown issue

[Ben Kelly]

Both fixes have stuck and are in the latest nightly.  Sorry for the
headaches here.

On Dec 1, 2017 1:15 PM, "Ben Kelly"  wrote:

> FYI, we have a fix identified for the late shutdown leak as well.  I will
> push them to inbound once the trees re-open.
>
> On Thu, Nov 30, 2017 at 11:18 PM, Ben Kelly  wrote:
>
>> Hi all,
>>
>> I just wanted to send a note about a bug in nightly which is leading to
>> delayed shutdowns.  Currently nightly is taking 5 to 10 seconds to shutdown.
>>
>> Bisection has shown this was introduced by my landing in:
>>
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1419536
>>
>> I have a fix here:
>>
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1420594
>>
>> But I discovered upon pushing the fix that at least one new leak has
>> crept in while shutdowns have been delayed.  I'm currently bisecting on try
>> to figure out what happened.  I am, however, worried about more leaks
>> appearing while I figure this out.
>>
>> If you are touching anything that might live until shutdown, please
>> include the patches from bug 1420594 to avoid the delayed shutdown issue.
>>
>> I'll send another mail once the fix lands and sticks.
>>
>> Thanks for your help.  Sorry for the trouble.
>>
>> Ben
>>
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: increased chance of new leaks due to delayed shutdown issue

[Ben Kelly]

FYI, we have a fix identified for the late shutdown leak as well.  I will
push them to inbound once the trees re-open.

On Thu, Nov 30, 2017 at 11:18 PM, Ben Kelly  wrote:

> Hi all,
>
> I just wanted to send a note about a bug in nightly which is leading to
> delayed shutdowns.  Currently nightly is taking 5 to 10 seconds to shutdown.
>
> Bisection has shown this was introduced by my landing in:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1419536
>
> I have a fix here:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1420594
>
> But I discovered upon pushing the fix that at least one new leak has crept
> in while shutdowns have been delayed.  I'm currently bisecting on try to
> figure out what happened.  I am, however, worried about more leaks
> appearing while I figure this out.
>
> If you are touching anything that might live until shutdown, please
> include the patches from bug 1420594 to avoid the delayed shutdown issue.
>
> I'll send another mail once the fix lands and sticks.
>
> Thanks for your help.  Sorry for the trouble.
>
> Ben
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

PSA: increased chance of new leaks due to delayed shutdown issue

[Ben Kelly]

Hi all,

I just wanted to send a note about a bug in nightly which is leading to
delayed shutdowns.  Currently nightly is taking 5 to 10 seconds to shutdown.

Bisection has shown this was introduced by my landing in:

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

I have a fix here:

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

But I discovered upon pushing the fix that at least one new leak has crept
in while shutdowns have been delayed.  I'm currently bisecting on try to
figure out what happened.  I am, however, worried about more leaks
appearing while I figure this out.

If you are touching anything that might live until shutdown, please include
the patches from bug 1420594 to avoid the delayed shutdown issue.

I'll send another mail once the fix lands and sticks.

Thanks for your help.  Sorry for the trouble.

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

service workers at risk for FF59 ESR

[Ben Kelly]

As you may be aware, we have disabled service workers in all ESR releases
since it was implemented.  At first this was because it was a big new
feature with security implications.  Then it was because we realized we
needed to rewrite a lot of code to properly support multi-e10s.  Back
porting security fixes across such a rewrite would be quite difficult.

Unfortunately it looks probable that we will need to disable service
workers in FF59 ESR as well.

There is some small hope that we will complete the service workers e10s
rewrite in FF59.  However, its also very likely this will not get done
given that the all hands, holiday, and company-wide extended PTO is taking
place in this cycle.

You may be asking, how close is the service worker e10s rewrite?  Have you
guys made any progress?

Let me provide a brief update of where we are:

- The main blocker has been bug 1293277 as it is necessary to provide
cross-process primitives we can use in ServiceWorkerManager and necko.
- I am now splitting off patches from the mega-patch in bug 1293277 and
landing them as they are reviewed.
- I expect bug 1293277 to 100% be done in FF59 and hopefully before the
holiday.
- We have also extricated the parent-side service worker interception code
from the main http cache path in bug 1391693.  This will make it easier to
refactor interception as it isolates us from some necko complexity.
- After bug 1293277 is done there will be additional work to use the
primitives throughout the ServiceWorkerManager to enable it to live
cross-process.
- We also need to update prototype work to do service worker launch and
event dispatch remotely from the parent process.
- We can then switch to performing all service worker interception and life
cycle management in the parent process.
- There will also probably be a reasonably long tail of stability and
performance issues to fix.  Fortunately we have a pretty good test suite to
avoid functional regressions.

At the end of the day we all would like this complete so we could enable in
ESR, but I think the amount of work remaining is challenging given the
holiday/all-hands cycle.  Realistically we should plan on service workers
being disabled in FF59 ESR.

Please let me know if you have any questions or concerns.  Thanks.

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

Re: mozilla-central now compiles with C++14

[Ben Kelly]

On Wed, Nov 15, 2017 at 8:44 PM, Nathan Froyd  wrote:

> * initialized lambda captures
>

I would like to use initialized lambda capture as I think it will allow
move-only objects to be used in lambdas like this:

UniquePtr uniqueThing = MakeUnique();
nsCOMPtr r = NS_NewRunnableFunction([uniqueThing =
Move(uniqueThing)] () {
  uniqueThing->Stuff();
});
thread->Dispatch(r.forget());

Thanks.

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

Re: Intent to ship: Retained Display Lists

[Ben Kelly]

On Tue, Oct 31, 2017 at 11:44 PM, Ben Kelly  wrote:

> Anecdotely this pref seems to make twitter much less janky on fennec on my
> Nexus 5x.
>

Actually, at least some retained display list code is disabled in non-e10s
because of a process type check.  Its unclear what flipping the pref does
or does not do on fennec at the moment.

I filed a bug for the process checks currently being used here:

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

Sorry for my confusion.

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

Re: Intent to ship: Retained Display Lists

[Ben Kelly]

On Oct 26, 2017 12:15 AM,  wrote:

On Monday, October 9, 2017 at 1:22:55 PM UTC+13, Matt Woodrow wrote:
> We're planning on landing the code for retaining display lists in 58,
> behind the pref layout.display.list.retain.
>

This has now landed in Nightly, and looks to be working really well.

We'd really appreciate some early feedback, so feel free to set
layout.display-list.retain=true and give it a try!


Anecdotely this pref seems to make twitter much less janky on fennec on my
Nexus 5x.

What's the plan for enabling by default?

Thanks!

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

Re: Default Rust optimization level decreased from 2 to 1

[Ben Kelly]

Would it be possible to have our profiling tools warn if about:compiler
optimization flags are not in about:buildconfig?

On Tue, Oct 31, 2017 at 3:39 PM, Jeff Muizelaar 
wrote:

> On Tue, Oct 31, 2017 at 3:21 PM, Gregory Szorc  wrote:
> > On Tue, Oct 31, 2017 at 12:02 PM, Jeff Muizelaar  >
> > wrote:
> >>
> >> As another piece of evidence in support opt-level=1 being the wrong
> >> default, Glenn also got bitten profiling with the wrong options.
> >> https://github.com/servo/webrender/issues/1817#issuecomment-340553613
> >
> >
> > It is "wrong" for the set of "people performing profiling." This set is
> > different from "people compiling Gecko." Which is different from "people
> who
> > actually need to compile Gecko." What I'm trying is the new default is
> "not
> > wrong" for a large set of people who aren't "people performing
> profiling."
>
> I say this a bit tongue-in-cheek, but given our big performance push,
> hopefully the set of people who aren't profiling is not that large.
>
> -Jeff
> ___
> 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

Intent to implement and ship PerformanceResourceTiming.workerStart

[Ben Kelly]

The PerformanceResourceTiming API has had a `workerStart` value specified
for a couple years now.  Its defined to represent the time when we trigger
a service worker FetchEvent.  It will be zero if service workers are not
involved.

https://bugzilla.mozilla.org/show_bug.cgi?id=1191943
https://w3c.github.io/resource-timing/#dom-performanceresourcetiming-workerstart

This will be available on all platforms.

I'm targeting it for release in FF58.

It is not behind a pref.  The property will always be there.  If SW are
disabled then the value will always be zero.

We have some work going on to try to display service worker timing
information in devtools:

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

Chrome has implemented workerStart for a while and there is a WPT test.

There are some spec/compat questions to clarify:

https://github.com/w3c/resource-timing/issues/118
https://github.com/w3c/resource-timing/issues/119

But I don't think those need to block shipping the property.  We can adjust
the values as the spec is clarified.  Also, one of the reasons I am
implementing this now is so we can enable the WPT test.  Our
PerformanceResourceTiming code is completely broken for SW in e10s right
now and we missed it because the test is not passing.  (This will be fixed
in FF58 as well.)

The WPT test is here and is being expanded slightly with our implementation:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker/resource-timing.https.html

Please let me know if there are questions or concerns.  Thanks.

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

Re: Containers graduation from Test Pilot - we still care about 57+

[Ben Kelly]

It disables multi-e10s.  Forced to one content process.

On Sep 27, 2017 12:58 PM, "Andrew McKay"  wrote:

Sorry, it disables e10s even though it has
true in the
install.rdf? That shouldn't be the case.

On 27 September 2017 at 07:14, Ben Kelly  wrote:
> On Mon, Sep 25, 2017 at 9:28 AM, Ben Kelly  wrote:
>
>> Thanks Jonathan.
>>
>> Also, it seems the link to the web extension version of the container
>> addon is broken above.  This one works for me:
>>
>> https://github.com/mozilla/multi-account-containers/releases/latest
>>
>
> Sorry, I guess this isn't a web extension yet.  Its a legacy addon and
> therefore disables multi-e10s.
> ___
> 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: Containers graduation from Test Pilot - we still care about 57+

[Ben Kelly]

On Mon, Sep 25, 2017 at 9:28 AM, Ben Kelly  wrote:

> Thanks Jonathan.
>
> Also, it seems the link to the web extension version of the container
> addon is broken above.  This one works for me:
>
> https://github.com/mozilla/multi-account-containers/releases/latest
>

Sorry, I guess this isn't a web extension yet.  Its a legacy addon and
therefore disables multi-e10s.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Containers graduation from Test Pilot - we still care about 57+

[Ben Kelly]

Thanks Jonathan.

Also, it seems the link to the web extension version of the container addon
is broken above.  This one works for me:

https://github.com/mozilla/multi-account-containers/releases/latest

On Sat, Sep 23, 2017 at 9:52 AM, Jonathan Kingston  wrote:

> Hi All,
>
> TL;DR - containers is going well, we made it a web extension and it will
> continue on addons.mozilla.org
>
> I just wanted to highlight that we have graduated our extension of Test
> Pilot Containers to be on AMO: https://addons.mozilla.org/en-GB/firefox/
> addon/multi-account-containers/
>
> We have had a few confused users about what is happening with containers.
>
> There is confusion around why the addon doesn't work in 57+ which is due to
> it being a legacy addon to support 56, the extension can now be fully
> WebExtensions after 57 and to install there you can use:
> https://github.com/
> mozilla/multi-account-containers/releases/latest the addons team is
> working
> on a fix for the website to allow Beta/Nightly users to download there.
>
> We have also removed in Nightly the ability to use the drawer icon which
> was native to nightly as it was a subset of features that were provided by
> the extension.
>
> To open a container:
> - Long press on the + button
> - Use the file menu (alt+f)
> - Use the extension above (ctrl+period)
> - Right click links with the context menu
>
> :bkelly suggested I email here in this bug: https://bugzilla.mozilla
> .org/show_bug.cgi?id=1402342 which provides further clarification.
>
> We have exciting posts coming soon to explain further, I just wanted to
> highlight we aren't dropping containers just clearing up experiences for
> when 57 is stable.
>
> Any user of >57 can actually install any container addon to enable
> containers:
> https://addons.mozilla.org/en-GB/firefox/collections/jkt/container-addons/
>
> In Beta and Stable releases containers is still disabled by default,
> installing a container addon will enable them. To reduce confusion here I
> have raised: https://bugzilla.mozilla.org/show_bug.cgi?id=1402608 to allow
> users of beta/stable to enable containers through about:preferences also.
>
> If in doubt feel free to reach out to: contain...@mozilla.com
>
> Thanks
> Jonathan
> ___
> 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: TabChild visibility

[Ben Kelly]

FWIW, our nsIDocument::VisibilityState() is updated when the docshell goes
active:

http://searchfox.org/mozilla-central/source/dom/base/nsIDocument.h#2855
http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#12504
http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#12552
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#6343


On Wed, Sep 20, 2017 at 3:18 PM, Mike Conley  wrote:

> Hello dev-platform,
>
> TL;DR: TabChild's don't seem to care about the nsIBaseWindow visibility
> attribute that they implement. In fact, they often lie about it.
>
> What should we do about that? What's the best way to detect and tell the
> TabChild that it's visible or invisible?
>
> Also, what's the difference between DocShell visibility and activeness?
>
>
> Longer story:
>
> In bug 1353013, I'm dealing with a permanent failure that I've tracked down
> to Marionette's listener.js causing focus to be pulled in the hidden,
> preloaded about:newtab that is running in the content process.
>
> With e10s disabled, nsGlobalWindow::Focus bails out early, because the
> function checks its visibility (via the nsIBaseWindow interface), and
> determines that it is invisible (because of this function[1] that sees if
> the frame is in the selected frame of a deck).
>
> With e10s enabled, visibility seems to be a lot less useful - TabChild
> implements nsIBaseWindow, and ignores attempts to set visibility[2] and
> always reports that it is visible[3]. This means that nsGlobalWindow::Focus
> decides that focus is indeed something that the preloaded about:newtab can
> do, and that causes my test failures.
>
> It seems to me that a TabChild always reporting that it's visible isn't
> amazing. For a browser window with a number of tabs open, it's the case
> that only one of those tabs really is visible, naturally; so the rest of
> those TabChild's are straight-up lying. I have no idea what the
> ramifications of that lying are. Perhaps we're doing more work than we need
> to because we're skipping some "don't do it if we're invisible"
> optimizations. I'm not sure.
>
> At any rate, it seems to me that the solution to my problem is to have the
> TabChild report the truth about whether or not it's visible. Using a sync
> IPC message to ask the parent is asking for trouble, so instead, I suspect
> we'll want the TabChild to be _told_ when it's made visible and invisible.
>
> What is the best way of doing that? Is there a clean way of having the
> nsFrameLoader / TabParent be told when they've been set as the active frame
> in a deck or not? Or is this something that the front-end needs to take
> responsibility for (like we do for DocShell active-ness) and we need to
> have tabbrowser.xml tell the TabChild that it has become visible?
>
> Or, alternatively, should I tie the activeness of the TabChild's DocShell
> to its visibility? That would be a change in behaviour between e10s and
> non-e10s; in non-e10s, it appears to be possible to have a non-visible but
> active DocShell (despite the documentation [4]). Perhaps somebody here can
> shed some light on the relationship (or lack thereof) between active-ness
> and visibility?
>
> Thanks,
>
> -Mike
>
> [1]:
> http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a41314524592
> 41010701e0/layout/generic/nsFrame.cpp#405-409
> [2]:
> http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a41314524592
> 41010701e0/dom/ipc/TabChild.cpp#936-942
> [3]:
> http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a41314524592
> 41010701e0/dom/ipc/TabChild.cpp#929-934
> [4]:
> http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a41314524592
> 41010701e0/docshell/base/nsIDocShell.idl#659-664
> ___
> 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

PSA: nsPipe3.cpp is now using diagnostic assertions

[Ben Kelly]

Hi all,

FYI, I just pushed a patch to nsPipe3.cpp that switches it to use
MOZ_DIAGNOSTIC_ASSERT() instead of the weaker assertions it used to use.
This class is used extensively throughout the browser and state problems
can manifest as intermittent hangs, crashes, and memory leaks.

For example, we got lucky that we caught this intermittent in a debug build
in automation:

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

We currently have no idea how many release build sessions have run into
this state inconsistency in the wild.  The diagnostic assertions will help
us catch these problems much sooner.

We debated whether to wait until the FF58 merge next week or not.
Switching to diagnostic assertions now may raise crash rates in the short
term.  We decided, however, that the ability to detect problems sooner
before they hit the release channel outweighed the downsides of the
increased nightly/dev-edition crashes.

The diagnostic assertions were pushed in:

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

Please let me know if you have any questions or concerns.

Thanks.

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

Re: Re-visiting the DOM tree depth limit in layout

[Ben Kelly]

On Wed, Sep 13, 2017 at 4:44 AM, Henri Sivonen  wrote:

> I suggest we do the following:
>
>  1) Change the HTML parser behave more like Blink's: Raise the limit
> to 513 elements deep and append elements violating the limit to the
> 512th element on the stack instead of dropping them. (Since the
> off-the-main-thread parser can't read from the DOM, the previous
> sentence is defined in terms of the stack and not in terms of looking
> up a parent as in Blink.) I already have the code for this.
>
>  2) Change the frame constructor limit to 1026. Rationale: This is
> notably larger than 513 by being 513 times two and just within what
> can be handled in the table-cell worst case on Mac and Linux with the
> existing run-time stack size limits.
>
>  3) Increase the run-time stack size on Windows such that 1026-deep
> display: table-cell doesn't overflow the stack.
>

Is it possible to have automated tests to verify these tunings remain valid
as the tree is changed over time?  Or maybe we already have those?

Thanks.

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

Re: indexedDB.open failing silently?

[Ben Kelly]

Can you have the user try in a fresh profile?

I think this behavior might occur if they used a profile in a newer version
of firefox (like nightly 57) and then try to take it back to an older
version (like release 55).  Database schemas can be updated in various
storage APIs on disk which prevent older versions of firefox from opening
the any storage for the origin.

On Tue, Sep 12, 2017 at 5:57 AM, Geoff  wrote:

> I'm trying to help some users of my extension with a problem.
> Unfortunately I can only offer suggestions of things to try and then wait
> for a reply.
>
> I asked one to put this code in the web console and tell me what happened:
>
> try {
> var request = indexedDB.open('thisIsATest', 1);
> request.onsuccess = console.log;
> request.onerror = console.error;
> request.onblocked = console.error;
> request.onupgradeneeded = console.log;
> 'finished';
> } catch(ex) {
> console.error(ex);
> }
>
> What you'd expect to see on the console is either "finished" and then one
> or more messages from the event handlers, or an error from the catch block.
>
> What they got instead was: undefined.
>
> This surely must mean that indexedDB.open is failing but not throwing an
> exception. I've tried disabling indexedDB, or making the appropriate
> directory read-only, but in both cases an error is reported (even if it is,
> helpfully, "UnknownError").
>
> So I'm stuck. What could be causing this, and what can I do about it?
>
> (If you want to chip in on the issue directly:
> https://github.com/darktrojan/newtabtools/issues/307)
>
> GL
> ___
> 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: disabled non-e10s tests on trunk

[Ben Kelly]

Joel,

Is there an easy way for me to run non-e10s tests on linux?  We often use
"t-style" try pushes where we only run tests on one platform.  Restricting
non-e10s to win7-debug means I either need to run tests on multiple
platforms or use windows for the "t-style".  I don't want to use windows
because it builds/runs much slower than linux.

Thanks.

Ben

On Fri, Aug 18, 2017 at 4:15 PM,  wrote:

> Yesterday I landed bug 1391371 which enabled non-e10s unittests on windows
> 7 debug.  A few tests had to be disabled in order to do this.  To keep
> track of what we did and each of the test suites to evaluate, I have filed
> bug 1391350.
>
> As a note we now have the following non-e10s tests running:
> windows7-debug: all trunk branches
> android opt/debug: all trunk branches (existing media, plain, reftest)
> linux64-jsdcov: mozilla-central (mochitest-plain/browser-chrome/devtools)
> ** this is a linux64 opt build and we use the jsdebugger to collect code
> coverage metrics- but we specifically run this in non-e10s mode.
>
> Please let me know if there are large areas that we have overlooked.
>
> Thanks!
> ___
> 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: Re: Firefox and clang-cl

[Ben Kelly]

On Thu, Sep 7, 2017 at 10:09 AM, Nathan Froyd  wrote:

> On Thu, Sep 7, 2017 at 10:04 AM, Ben Kelly  wrote:
> > On Mon, Aug 14, 2017 at 10:44 AM, Tristan Bourvon 
> > wrote:
> >
> >> Here's the RFC of the overflow builtins:
> >> http://clang-developers.42468.n3.nabble.com/RFC-Introduce-
> >> overflow-builtins-td3838320.html
> >> Along with the tracking issue: https://bugs.llvm.org/show_
> bug.cgi?id=12290
> >> And the patch:
> >> https://github.com/llvm-mirror/clang/commit/
> 98d1ec1e99625176626b0bcd44cef7
> >> df6e89b289
> >>
> >> There's also another patch that was added on top of this one which adds
> >> more overflow builtins:
> >> https://github.com/llvm-mirror/clang/commit/
> c41c63fbf84cc904580e733d1123d3
> >> b03bb5584c
> >>
> >> It seems clear that this optimization could bring big performance
> >> improvements on hot functions. It could also reduce binary size
> >> substantially (we're talking about 14->5 instructions in their case).
> >>
> >
> > Do we have a bug filed to investigate these overflow builtins?  Should we
> > file one?
>
> There is bug 1356936 for mozilla::CheckedInt; I don't know how many
> saturating-style arithmetic implementations we have in the tree, or
> whether similar bugs exist for those.
>

I guess my impression was this would be something we would want the jit to
emit in its bytecode.  But maybe I don't fully understand.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Re: Firefox and clang-cl

[Ben Kelly]

On Mon, Aug 14, 2017 at 10:44 AM, Tristan Bourvon 
wrote:

> Here's the RFC of the overflow builtins:
> http://clang-developers.42468.n3.nabble.com/RFC-Introduce-
> overflow-builtins-td3838320.html
> Along with the tracking issue: https://bugs.llvm.org/show_bug.cgi?id=12290
> And the patch:
> https://github.com/llvm-mirror/clang/commit/98d1ec1e99625176626b0bcd44cef7
> df6e89b289
>
> There's also another patch that was added on top of this one which adds
> more overflow builtins:
> https://github.com/llvm-mirror/clang/commit/c41c63fbf84cc904580e733d1123d3
> b03bb5584c
>
> It seems clear that this optimization could bring big performance
> improvements on hot functions. It could also reduce binary size
> substantially (we're talking about 14->5 instructions in their case).
>

Do we have a bug filed to investigate these overflow builtins?  Should we
file one?

Even if we can't use them on all platforms yet, it might be a nice win on
mac where we lack other optimizations like PGO right now.

Thanks.

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

Re: Intent to ship: Abort API

[Ben Kelly]

On Wed, Sep 6, 2017 at 12:27 PM, Andrea Marchesini 
wrote:

> Abort API and the Fetch() integration should be enabled by default in the
> next nightly.
>

To clarify, its already enabled in nightly, but the patch to let it ride
the trains is now in inbound.

Thanks for implementing this Andrea!

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

Re: Quantum Flow Engineering Newsletter #22

[Ben Kelly]

On Fri, Sep 1, 2017 at 10:06 AM, Ben Kelly  wrote:

> On Fri, Sep 1, 2017 at 1:59 AM, Ehsan Akhgari 
> wrote:
>
>>- Mike Conley made it so that background tabs are “warmed up” when
>>hovering the mouse cursor over them
>><https://bugzilla.mozilla.org/show_bug.cgi?id=1385453>. This should
>>improve tab switching perceived performance.
>>
>
> I just want to highlight that telemetry shows that the mean tab switch
> time dropped from ~30ms to ~3ms:
>
> https://mzl.la/2grbPB2
>

Sorry, this is median. I incorrectly said mean.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Quantum Flow Engineering Newsletter #22

[Ben Kelly]

On Fri, Sep 1, 2017 at 1:59 AM, Ehsan Akhgari 
wrote:

>- Mike Conley made it so that background tabs are “warmed up” when
>hovering the mouse cursor over them
>. This should
>improve tab switching perceived performance.
>

I just want to highlight that telemetry shows that the mean tab switch time
dropped from ~30ms to ~3ms:

https://mzl.la/2grbPB2

An order of magnitude improvement! \o/
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: Abort API

[Ben Kelly]

On Tue, Aug 29, 2017 at 9:39 AM, Ben Kelly  wrote:

> On Tue, Aug 29, 2017 at 2:05 AM, Andrea Marchesini <
> amarches...@mozilla.com> wrote:
>
>> Abort API is already part of the DOM spec and I would like to enable it by
>> default everywhere in our codebase (dom.abortController.enabled). Abort +
>> Fetch integration is not part of the spec yet. There is a read-ish
>> pull-request, but as soon as it is merged, I would like to enable the
>> second pref as well (dom.abortController.fetch.enabled).
>>
>
> I don't understand why we would potentially ship these separately.  AFAICT
> the abort API is only really useful if its integrated into something like
> fetch().  Currently the Fetch API is the only part of the platform that
> uses it.
>
> Also, we have seen people do thing like feature detect for ReadableStream
> and assume Response.body can be a ReadableStream.  This is why we are not
> shipping the js ReadableStream until the fetch body stream can ship as
> well.  I worry that people would do a similar kind of feature detection
> with Abort API.
>
> Also, while the fetch+abort spec change has not landed yet, we pretty much
> have consensus.  There are WPT tests in the tree already.  I don't think
> this is what is blocking shipping the fetch+abort code.  To me its a bigger
> issue that we still don't pass many of those WPT tests.  If we passed the
> tests then I think we could reasonably ship it given the current level of
> consensus on the spec.
>

I spoke with Andrea and Jake (the spec author on this feature) a bit this
morning.  Our current test failures are all reasonable for different
reasons:

https://bugzilla.mozilla.org/show_bug.cgi?id=1394085#c4

So I would just like to see bug 1394102 land before we let abortable fetch
ride the trains.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: Abort API

[Ben Kelly]

On Tue, Aug 29, 2017 at 2:05 AM, Andrea Marchesini 
wrote:

> Abort API is already part of the DOM spec and I would like to enable it by
> default everywhere in our codebase (dom.abortController.enabled). Abort +
> Fetch integration is not part of the spec yet. There is a read-ish
> pull-request, but as soon as it is merged, I would like to enable the
> second pref as well (dom.abortController.fetch.enabled).
>

I don't understand why we would potentially ship these separately.  AFAICT
the abort API is only really useful if its integrated into something like
fetch().  Currently the Fetch API is the only part of the platform that
uses it.

Also, we have seen people do thing like feature detect for ReadableStream
and assume Response.body can be a ReadableStream.  This is why we are not
shipping the js ReadableStream until the fetch body stream can ship as
well.  I worry that people would do a similar kind of feature detection
with Abort API.

Also, while the fetch+abort spec change has not landed yet, we pretty much
have consensus.  There are WPT tests in the tree already.  I don't think
this is what is blocking shipping the fetch+abort code.  To me its a bigger
issue that we still don't pass many of those WPT tests.  If we passed the
tests then I think we could reasonably ship it given the current level of
consensus on the spec.


> Other vendors:
> Chromium https://bugs.chromium.org/p/chromium/issues/detail?id=750599
> I haven't found anything about Edge and Safari.
>

Edge has marked their issue for this "fixed":

https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/13009916/

It hasn't shipped yet, of course.


> Estimated target date: Firefox 58.
>

If you are asking to enable the Abort API by default now, wouldn't that put
it in FF57?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New string types: nsAutoStringN<> and nsAutoCStringN<>

[Ben Kelly]

On Mon, Aug 21, 2017 at 10:00 AM, Ben Kelly  wrote:

> Should that be `mStorage[N + 1]`?
>

Maybe not since things like NSID_LENGTH include the null pointer on their
end.  Sorry for my confusion.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: New string types: nsAutoStringN<> and nsAutoCStringN<>

[Ben Kelly]

On Sun, Aug 20, 2017 at 6:35 PM, Nicholas Nethercote  wrote:

> Hi,
>
> For a long time we have had types nsAutoString and nsAutoCString which are
> strings with 64 chars of inline storage. They are good for holding short
> strings, most often on the stack, because they avoid the need to heap
> allocate
> a char buffer.
>
> I recently landed patches (bug 1386103) that introduce nsAutoStringN and
> nsAutoCStringN. These are like the existing types but their length is a
> template parameter. So if you want an nsString with 128 chars of inline
> storage, you'd use nsAutoStringN<128>. If you want an nsCString with enough
> inline storage to store an nsID you'd use nsAutoCStringN.
>
> nsAutoString and nsAutoCString have been redefined as typedefs for
> nsAutoStringN<64> and nsAutoCStringN<64>, respectively.
>

First, let me say this is a great addition.  Thanks!

I do have a question, though.  My impression was that something like
nsAutoCString stored its data in a null terminated string.  So you can do
things like:

  nsAutoCString someValue;
  printf_stderr("value = %s\n", someValue.get());

Does nAutoCStringN also store its value null-terminated?  Is that null
somehow accounted for in the storage?  I don't see where that is done:

http://searchfox.org/mozilla-central/source/xpcom/string/nsTString.h#664

Should that be `mStorage[N + 1]`?

Thanks.

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

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Wed, Aug 16, 2017 at 2:32 PM, Joel Maher  wrote:

> Thanks everyone for chiming in here.  I see this isn't as simple as a
> binary decision and to simplify things, I think turning on all non-e10s
> tests that were running for windows7-debug would give us reasonable
> coverage and ensure that users on our most popular OS (and focus for 57)
> have a stable experience if they choose to go without e10s.
>
> Instead of a date to turn things off, I would like to just focus on each
> framework one at a time and ideally in a few months we would have a clear
> set of requirements (in the form of bugs) to resolve before turning off the
> specific non-e10s tests.
>
> If there is a different configuration other than windows7-debug I would
> like to hear about it.
>

Thank you Joel.

My only thought about windows7-debug is that android is a variant of
linux.  Running a linux platform might be closer to android behavior.  But
I don't have a known specific difference in mind.

It would also be nice to opt and debug since things do differ between them,
but I'll take what I can get.

Thanks.

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

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Tue, Aug 15, 2017 at 4:43 PM, Joel Maher  wrote:

> This is a discussion about tests in e10s mode, not WPT on Android.
>

Yes.  And android is our last tier 1 platform that requires non-e10s.  I
think that makes it relevant to the discussion.


>
> What specific coverage are we missing by not running WPT in non-e10s mode
> on desktop?  When can we ensure we have that coverage in e10s mode?  I
> assume this is just making sure the tests that we have disabled on e10s
> mode need to get fixed.
>

Some features are implemented completely differently in e10s vs non-e10s
mode.  The main one I am aware of is service workers.  If you turn on
non-e10s WPT tests there will be regressions.

Don't get me wrong.  I'd prefer not to deal with non-e10s either.  But its
*absolutely false* to say we don't need to support it right now because its
required for a tier 1 platform.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Tue, Aug 15, 2017 at 4:37 PM, Joel Maher  wrote:

> All of the above mentioned tests are not run on Android (well
> mochitest-media is to some degree).  Is 4 months unreasonable to fix the
> related tests that do not run in e10s?  Is there another time frame that
> seems more reasonable?
>

Last I checked it was your team that told me WPT on android was not an
immediate priority.  The WPT harness itself does not run there.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Tue, Aug 15, 2017 at 4:29 PM,  wrote:

> While there are many tests which individually are disabled or lacking
> coverage, these test suites have no non-e10s coverage:
> * web-platform-tests
> * browser-chrome
> * devtools
> * jsreftests
> * mochitest-webgl, mochitest-gpu, mochitest-media
> * reftest un-accel
>
> I would propose running these above tests on windows7-opt (we don't have
> these running yet in windows 10, although we are close), and only running
> specific tests which are not run in e10s mode, turning them off December
> 29th, 2017.
>
> Keep in mind we have had many years to get all our tests running in e10s
> mode and we have known since last year that Firefox 57 would be the first
> release that we ship e10s by default to our users- my proposal is a 4 month
> temporary measure to let test owners finish ensuring their tests are
> running in e10s mode.
>

WPT tests do not run on android.  Unless we can get WPT tests running on
android, I don't think dropping all non-e10s coverage for them is
reasonable.  I don't know of any plan to move android to e10s by Dec 29.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: 64-bit Firefox progress report: 2017-07-18

[Ben Kelly]

Chris,

Do you know who controls this blog post?

https://blog.mozilla.org/firefox/firefox-64-default-64-bit-windows/

The chart is really misleading.  What does the vertical bar chart for
"security" even mean?  As noted on twitter:

https://twitter.com/kylealden/status/897222041476005888

The bar chart for "crashes" at least has an intelligible y-axis, but it
seems wrong.  The chart shows more like a 60% improvement instead of a 39%
improvement.

Perhaps we should just replace this graphic with an up-arrow for security
and a down-arrow for crashes?

Anyway, sorry to be pedantic, but misleading charts are kind of a pet peeve.

Thanks.

Ben

On Wed, Jul 19, 2017 at 2:38 AM, Chris Peterson 
wrote:

> We are on track to make 64-bit Firefox the default build for Win64 OS,
> bringing improved ASLR and fewer OOM crashes to the 70% of Windows Firefox
> users running Win64.
>
> PLANS:
>
> * In Firefox 55 (August 8), the Windows stub installer will default to
> 64-bit Firefox for eligible users (Win64 and 2+ GB RAM).
>
> * In Firefox 56 (September 26), we will migrate existing eligible 32-bit
> Firefox users to 64-bit. About 70% of Windows Firefox users currently run
> 32-bit Firefox build on Win64. Nearly all of these users can be migrated to
> 64-bit Firefox.
>
> Our Win64 project wiki has more details about the long road to making
> 64-bit Firefox the default:
>
> https://wiki.mozilla.org/Firefox/Win64
>
> PROGRESS:
>
> * David Parks fixed the insidious Flash video streaming bug affecting
> Xfinity and MLB! (bug 1334803)
>
> * Bob Owen fixed the sandbox issue that prevented 64-bit Firefox from
> being run from a network-mounted drive! (bug 1377555)
>
> * Some highlights from week 2 of our Firefox 54 experiment comparing 32-
> and 64-bit Firefox users:
>
> - About 8% of Windows users have <= 2 GB RAM!
>
> - User retention and engagement metrics (session length, # of pages, # of
> tabs) are about the same for 32- and 64-bit Firefox users, regardless of
> memory size.
>
> - 64-bit content process crash rate is about the same as 32-bit for users
> with 2 GB and about 20% less with 2+ GB!
>
> - 64-bit browser process crash rate is about the same as 32-bit for users
> with 2 GB and about 20% less with 2+ GB!
>
> - 64-bit plugin process crash rate is about 50% less than 32-bit for users
> with 2 GB and 80% less with 2+ GB!
>
> We are still considering whether 64-bit Firefox should have a default
> minimum memory requirement. As a placeholder value, Firefox 55 currently
> requires 2+ GB, but based on the results of our experiment, we may remove
> the minimum memory requirement. Microsoft's minimum memory require for
> Win64 itself is 2 GB, so anyone running Win64 with less than 2 GB is having
> a bad time.
> ___
> 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: Linking with lld instead of ld/gold

[Ben Kelly]

On Sun, Aug 13, 2017 at 5:08 PM, Sylvestre Ledru 
wrote:

> To use it, you should have a clang >= 4.0 installed and lld installed
> on the system.
> clang is in charge of the LLD detection with its option -fuse-ld=lld
> (this option is also supported by gcc since version 6). Clang will
> look for the same version of LLD available on the system.
>

The clang installed by ./mach bootstrap in .mozbuild is 3.9.0.  Is there
any plan to update that to >= 4.0?

Thanks.

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

Re: Response.body streams landing on trunk, default off

[Ben Kelly]

FYI, the Fetch API side of streams has landed and is now in nightly.
Please test and file bugs.  Thanks!

Ben

On Thu, Aug 10, 2017 at 11:29 PM, Ben Kelly  wrote:

> Hi all,
>
> As some of you may know :till and :baku have been working hard to
> implement ReadableStream support.  Till landed the js bits in bug 1272697:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1272697
>
> Andrea has been working on the DOM integration with Fetch API in bug
> 1128959:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1128959
>
> This second bug is now on inbound and looks like its going to stick
> (famous last words...).  Hopefully it will be in trunk tomorrow and in
> Saturday's nightly.
>
> Please note that streams will be disabled by default for now.  There are a
> few more issues to address before we can enable it by default.
>
> If you would like to test Response.body please enable these two prefs:
>
>   javascript.options.streams
>   dom.streams.enabled
>
> Please test and file any bugs you might find. I've done some local testing
> on a few sites I'm aware of that use streams and so far I have not found
> any functional problems.
>
> Many thanks to Till and Andrea for their hard work on streams!
>
> Ben
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Firefox and clang-cl

[Ben Kelly]

On Mon, Aug 14, 2017 at 6:11 AM, Till Schneidereit <
t...@tillschneidereit.net> wrote:

> On Mon, Aug 14, 2017 at 9:27 AM, Julian Seward  wrote:
>
> > On 13/08/17 03:40, Ehsan Akhgari wrote:
> > > As you may have heard by now, Chromium has started to switch their
> > Windows
> > > builds to use clang-cl instead of MSVC [1].  This has improved their
> > > Speedometer v2 benchmark score on x86 (but not on x86-64) by about 30%
> > > according to AWFY [2].  [..]
> >
> > Do we have any insight into why the Clang version is so much faster?
> > 30% strikes me as a large difference for two supposedly mature optimizing
> > compilers.  And stranger still that it applies only for the 32-bit case.
> > So I'm curious to know what's changed.
> >
>
> AFAICT, the real change is about 19%: shortly before the jump to ~103,
> their score regressed from 86 to 78. I think using 86 as the baseline makes
> much more sense. 19% is obviously still a substantial improvement from a
> compiler change.
>

Google ran a bisect on the regression and improvements.  Here is some
explanation of the regression:

https://bugs.chromium.org/p/chromium/issues/detail?id=749359#c4

The comment suggests that the change would see an improvement in clang
since the code would then use class "overflow builtins".

Its hard to tell if all of the clang-on-windows improvement is due to this
same set of code or not.

The bisects were run from this issue:

https://bugs.chromium.org/p/chromium/issues/detail?id=750672#c12

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

Response.body streams landing on trunk, default off

[Ben Kelly]

Hi all,

As some of you may know :till and :baku have been working hard to implement
ReadableStream support.  Till landed the js bits in bug 1272697:

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

Andrea has been working on the DOM integration with Fetch API in bug
1128959:

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

This second bug is now on inbound and looks like its going to stick (famous
last words...).  Hopefully it will be in trunk tomorrow and in Saturday's
nightly.

Please note that streams will be disabled by default for now.  There are a
few more issues to address before we can enable it by default.

If you would like to test Response.body please enable these two prefs:

  javascript.options.streams
  dom.streams.enabled

Please test and file any bugs you might find. I've done some local testing
on a few sites I'm aware of that use streams and so far I have not found
any functional problems.

Many thanks to Till and Andrea for their hard work on streams!

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

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Tue, Aug 8, 2017 at 5:18 PM, Ben Kelly  wrote:

> On Tue, Aug 8, 2017 at 5:12 PM,  wrote:
>
>> In bug 1386689, we have turned them off.  There was some surprise in
>> doing this and some valid concerns expressed in comments in the bug.  Given
>> that, I thought we should bring this information to a wider audience on
>> dev.platform so more developers are aware of the change.
>>
>> While we get some advantages to not running duplicated tests (faster try
>> results, less backlogs, fewer intermittent failures) there might be
>> compelling reasons to run some tests in e10s based on specific coverage.
>> With that said, I would like to say that any tests we turn on as non-e10s
>> must have a clearly marked end date- as in this is only a temporary measure
>> while we schedule work in to gain this coverage fully with e10s tests.
>>
>
> If we have an android test disabled (a lot I think), then we should
> consider continuing to run the test in non-e10s on desktop linux.  Having
> more real devices to run android tests on would probably reduce the number
> of tests that are disabled.  The emulator is extremely slow and not
> representative of real hardware.
>

BTW, we have a large corpus of tests that don't run on android at all:
WPT.  Increasingly over time features are only covered by WPT tests.

I think we should keep at least non-e10s running on linux or linux64 for
now until we can improve our android test coverage or move android to e10s.

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

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Tue, Aug 8, 2017 at 5:18 PM, Ben Kelly  wrote:

> On Tue, Aug 8, 2017 at 5:12 PM,  wrote:
>
>> While we get some advantages to not running duplicated tests (faster try
>> results, less backlogs, fewer intermittent failures) there might be
>> compelling reasons to run some tests in e10s based on specific coverage.
>> With that said, I would like to say that any tests we turn on as non-e10s
>> must have a clearly marked end date- as in this is only a temporary measure
>> while we schedule work in to gain this coverage fully with e10s tests.
>>
>
> If we have an android test disabled (a lot I think), then we should
> consider continuing to run the test in non-e10s on desktop linux.  Having
> more real devices to run android tests on would probably reduce the number
> of tests that are disabled.  The emulator is extremely slow and not
> representative of real hardware.
>

Sorry to self-reply, but the best solution would of course be to move
android to a single content process e10s model.  Then we could truly remove
non-e10s.  Obviously that is probably not easy to do, though...

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

Re: disabled non-e10s tests on trunk

[Ben Kelly]

On Tue, Aug 8, 2017 at 5:12 PM,  wrote:

> As Firefox 57 is on trunk, we are shipping e10s by default.  This means
> that our primary support is for e10s.  As part of this, there is little to
> no need to run duplicated tests in non-e10s and e10s mode.
>

We still run android in non-e10s mode, right?


> In bug 1386689, we have turned them off.  There was some surprise in doing
> this and some valid concerns expressed in comments in the bug.  Given that,
> I thought we should bring this information to a wider audience on
> dev.platform so more developers are aware of the change.
>
> While we get some advantages to not running duplicated tests (faster try
> results, less backlogs, fewer intermittent failures) there might be
> compelling reasons to run some tests in e10s based on specific coverage.
> With that said, I would like to say that any tests we turn on as non-e10s
> must have a clearly marked end date- as in this is only a temporary measure
> while we schedule work in to gain this coverage fully with e10s tests.
>

If we have an android test disabled (a lot I think), then we should
consider continuing to run the test in non-e10s on desktop linux.  Having
more real devices to run android tests on would probably reduce the number
of tests that are disabled.  The emulator is extremely slow and not
representative of real hardware.

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

Re: sccache as ccache

[Ben Kelly]

On Wed, Aug 2, 2017 at 12:26 PM, Ben Kelly  wrote:

> On Wed, Jul 26, 2017 at 9:05 AM, Ted Mielczarek 
> wrote:
>
>> Yesterday I published sccache 0.2 to crates.io, so you can now `cargo
>> install sccache` and get the latest version (it'll install to
>> ~/.cargo/bin).
>>
>
> I tried this on my linux build machine today and got:
>
> error: failed to run custom build command for `openssl-sys v0.9.15`
> process didn't exit successfully: `/tmp/cargo-install.
> FAs9llrjaqDW/release/build/openssl-sys-a543e0ede317714a/build-script-build`
> (exit code: 101)
> --- stdout
> cargo:rerun-if-env-changed=OPENSSL_LIB_DIR
> cargo:rerun-if-env-changed=OPENSSL_INCLUDE_DIR
> cargo:rerun-if-env-changed=OPENSSL_DIR
> run pkg_config fail: "`\"pkg-config\" \"--libs\" \"--cflags\" \"openssl\"`
> did not exit successfully: exit code: 1\n--- stderr\nPackage openssl was
> not found in the pkg-config search path.\nPerhaps you should add the
> directory containing `openssl.pc\'\nto the PKG_CONFIG_PATH environment
> variable\nNo package \'openssl\' found\n"
>
> --- stderr
> thread 'main' panicked at '
>
> Could not find directory of OpenSSL installation, and this `-sys` crate
> cannot
> proceed without this knowledge. If OpenSSL is installed and this crate had
> trouble finding it,  you can set the `OPENSSL_DIR` environment variable
> for the
> compilation process.
>
> If you're in a situation where you think the directory *should* be found
> automatically, please open a bug at https://github.com/sfackler/
> rust-openssl
> and include information about your system as well as this message.
>
> $HOST = x86_64-unknown-linux-gnu
> $TARGET = x86_64-unknown-linux-gnu
> openssl-sys = 0.9.15
>
> ', /home/bkelly/.cargo/registry/src/github.com-
> 1ecc6299db9ec823/openssl-sys-0.9.15/build.rs:198
> note: Run with `RUST_BACKTRACE=1` for a backtrace.
>
> Build failed, waiting for other jobs to finish...
> error: failed to compile `sccache v0.2.0`, intermediate artifacts can be
> found at `/tmp/cargo-install.FAs9llrjaqDW`
>

Looks like I needed to run this as a pre-req on ubuntu:

sudo apt-get install pkg-config libssl-dev
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: sccache as ccache

[Ben Kelly]

On Wed, Jul 26, 2017 at 9:05 AM, Ted Mielczarek  wrote:

> Yesterday I published sccache 0.2 to crates.io, so you can now `cargo
> install sccache` and get the latest version (it'll install to
> ~/.cargo/bin).
>

I tried this on my linux build machine today and got:

error: failed to run custom build command for `openssl-sys v0.9.15`
process didn't exit successfully:
`/tmp/cargo-install.FAs9llrjaqDW/release/build/openssl-sys-a543e0ede317714a/build-script-build`
(exit code: 101)
--- stdout
cargo:rerun-if-env-changed=OPENSSL_LIB_DIR
cargo:rerun-if-env-changed=OPENSSL_INCLUDE_DIR
cargo:rerun-if-env-changed=OPENSSL_DIR
run pkg_config fail: "`\"pkg-config\" \"--libs\" \"--cflags\" \"openssl\"`
did not exit successfully: exit code: 1\n--- stderr\nPackage openssl was
not found in the pkg-config search path.\nPerhaps you should add the
directory containing `openssl.pc\'\nto the PKG_CONFIG_PATH environment
variable\nNo package \'openssl\' found\n"

--- stderr
thread 'main' panicked at '

Could not find directory of OpenSSL installation, and this `-sys` crate
cannot
proceed without this knowledge. If OpenSSL is installed and this crate had
trouble finding it,  you can set the `OPENSSL_DIR` environment variable for
the
compilation process.

If you're in a situation where you think the directory *should* be found
automatically, please open a bug at https://github.com/sfackler/rust-openssl
and include information about your system as well as this message.

$HOST = x86_64-unknown-linux-gnu
$TARGET = x86_64-unknown-linux-gnu
openssl-sys = 0.9.15

',
/home/bkelly/.cargo/registry/src/github.com-1ecc6299db9ec823/openssl-sys-0.9.15/
build.rs:198
note: Run with `RUST_BACKTRACE=1` for a backtrace.

Build failed, waiting for other jobs to finish...
error: failed to compile `sccache v0.2.0`, intermediate artifacts can be
found at `/tmp/cargo-install.FAs9llrjaqDW`
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to remove: sensor APIs

[Ben Kelly]

On Mon, Jul 24, 2017 at 5:10 AM, Anne van Kesteren  wrote:

> * Device orientation
>

Isn't this one required to build a decent web experience on mobile for some
sites?  It seems pretty common on mobile to adjust the UX based on whether
the device is in portrait/landscape orientation.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Keyboard APZ has landed on Inbound

[Ben Kelly]

On Sat, Jul 22, 2017 at 2:05 AM, Ryan Hunt  wrote:

> Keyboard APZ can't be used in every case. Currently it's disabled in the
> presense of key event listeners, as they can preventDefault scrolling and
> that
> is a non-negotiable part of the web.
>

Do we do keyboard APZ if the event listener is passive:true?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: 64-bit Firefox progress report: 2017-07-18

[Ben Kelly]

On Jul 19, 2017 8:57 PM, "Mike Hommey"  wrote:

On Wed, Jul 19, 2017 at 08:48:45PM -0400, Ben Kelly wrote:
> On Jul 19, 2017 6:20 PM, "Mike Hommey"  wrote:
>
> > What would be the rationale behind this choice?
>
> Smaller memory footprint, which, you'll admit, when you're on a machine
> with (less than) 2GB RAM, makes a difference.
>
>
> I thought we had data that showed OOM (small) due to VM fragmentation
still
> outweighed OOM (large) on these machines.  If that is the case 64-bit is
> strictly better.

I don't believe you can exhaust the address space and have VM
fragmentation be an actual problem when you have less than 2GB RAM. Of
if you do, everything is slow to a crawl before that happens.


I don't understand why that would be, but if so it should show in
crashstats as fewer small OOMs on these devices.  Does the data actually
show that?

Also, that tells nothing about people that never hit OOM (a lot of
people even close their browser well before that could happen).


You're saying the size difference between 32-bit and 64-bit is so great
users will start manually shutting down?  I find that hard to believe.  If
they are shutting down apps to save memory they will probably do it with
both 32-bit and 64-bit.  Again, though, in theory we have usage hour
telemetry we could look at to confirm (although maybe only for beta?).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: 64-bit Firefox progress report: 2017-07-18

[Ben Kelly]

On Jul 19, 2017 6:20 PM, "Mike Hommey"  wrote:

> What would be the rationale behind this choice?

Smaller memory footprint, which, you'll admit, when you're on a machine
with (less than) 2GB RAM, makes a difference.


I thought we had data that showed OOM (small) due to VM fragmentation still
outweighed OOM (large) on these machines.  If that is the case 64-bit is
strictly better.

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

Re: Phabricator Update, July 2017

[Ben Kelly]

On Mon, Jul 17, 2017 at 12:27 PM, Gregory Szorc  wrote:

> If the bug is only serving as an anchor to track code review, then the
> question we should be asking is "do we even need a bug."
>

In my experience the answer to this is "yes, we need a bug".  I very rarely
have a one-to-one mapping between bugs and review requests.  Sometimes
there are many reviews per bug.  Sometimes there are no reviews in a bug,
but it still contains useful information.  All the meta information around
bug blockers/dependents is orthogonal to reviews.

If you want to get rid of bugzilla in favor of your new tool, you are
really creating a new bug tracker with a built-in review tool, IMO.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Phabricator Update, July 2017

[Ben Kelly]

On Jul 14, 2017 6:27 PM, "Mike Hommey"  wrote:

On Fri, Jul 14, 2017 at 01:00:51PM -0400, Ben Kelly wrote:
> I know feedback was collected, but maybe not from this group.

Feedback was collected from a selected set of the people who do the most
reviews. I'm one of them. I don't think I'm breaking any secret by
saying that the list of people who received the consultation email at
the same time as I did were maire, mconley, bz, ehsan, smaug, billm and
mattn.


That's good!

That was a generic consultation as to what a review tool should provide
or not, before any decision was made on a specific product.

I guess there should be another round for implementation details, if
there isn't one already.


Yea, getting buy-in and consensus for a solution from those heavy reviewers
would probably increase the chance of success.

Thanks.

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

Re: Phabricator Update, July 2017

[Ben Kelly]

Also a random reply.

I think this kind of effort is more likely to be successful if it gets
input and buy-in from the key stakeholders.  In this case that would be the
most frequent reviewers.

It would be nice to run a bugzilla query to find the top 10 or 20
reviewers.  Talk to these folks, solve their problems, and get buy-in for a
solution.  The rest of the org will likely follow their lead.

Right now, though, I talk to people who spend all day reviewing and they
tell me no one ever talked to them.

I know feedback was collected, but maybe not from this group.

Anyway, just a suggestion.

Ben

On Jul 14, 2017 11:34 AM, "Milan Sreckovic"  wrote:

> Replying in general, to a random message :)
>
> I don't have the numbers, but I imagine reviews are happening in the
> hundreds every day (if we land almost 300 patches.)  So, I wouldn't expect
> the conversation about adding/removing/changing tools involved in reviews
> to be any less complicated, passionate and involved as what we're having
> here :)
>
> I believe Mark is putting together an e-mail to give us a better place to
> continue these conversations; something with meta bugs for "enable
> phabricator", "disable splinter", etc.  This should let us track the issues
> that are raised, discuss decisions as to which of those should be blockers,
> and have a better record of what actually gets resolved and how.
>
> This should leave us *just* with the decision making and communication
> rollout issues that were raised; I know we are continuously discussing
> those and trying to get better at it, but it is a separate conversation
> from the technical issues, so it's probably OK that it remains separate.
>
>
> On 13-Jul-17 15:41, Randell Jesup wrote:
>
>> To answer the other part of your question, MozReview will be disabled for
> active use across the board, but it is currently used by a small
> number of
> projects.  Splinter will be disabled on a per-product basis, as there
> may be
> some projects that can't, won't, or shouldn't be migrated to
> Phabricator.
>
 Splinter is still a nice UI to look at patches already attached to bugs.
 Please don't disable it.

>>> excellent point; thanks for that feedback.
>>>
>>> instead of disabling splinter for phabricator backed products, we could
>>> make it a read-only patch viewer.
>>>
>> I would consider it a massive fail if we disabled at least read-only
>> splinter viewing of patches.  As noted before.  let's make sure we
>> don't lose things we agreed on as issues.
>>
>>
> --
> - Milan (mi...@mozilla.com)
>
> ___
> 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: Phabricator Update, July 2017

[Ben Kelly]

On Wed, Jul 12, 2017 at 11:27 AM, Byron Jones  wrote:

> But indeed having also the patches in bugzilla would be good.
>>
> no, it would be bad for patches to be duplicated into bugzilla.  we're
> moving from bugzilla/mozreview to phabricator for code review, duplicating
> phabricate reviews back into the old systems seems like a step backwards
> (and i'm not even sure what the value is of doing so).
>

I find this a strange argument.  We don't know how successful phrabricator
is going to be.  The last attempt to switch review tools seems to be
getting killed.  I think its somewhat reasonable to be skeptical of further
review tool changes.

Also, I have to say the whole phabricator thing has been kind of presented
as a fait accompli. As someone who continues to use splinter on a daily
basis I'm 1) skeptical and 2) kind of unhappy.

Maybe phabricator will be great, but I'll believe it when I see it.  Please
don't force future data and workflow into an as-yet unproven tool.

Thanks.

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

Re: Phabricator Update, July 2017

[Ben Kelly]

On Tue, Jul 11, 2017 at 11:49 PM, Martin Thomson  wrote:

> On Wed, Jul 12, 2017 at 1:34 PM, Byron Jones  wrote:
> > instead of disabling splinter for phabricator backed products, we could
> make
> > it a read-only patch viewer.
>
> Given the number of bugs that exist with patches attached, that would
> be greatly appreciated.  I would also assume that attaching patches
> won't stop completely either.
>

It would be nice if patch information was always contained within the bug.
Scattering it across tools makes it harder to figure out happened later
when you are researching why changes were made.

Consider that we are talking about "turning off" mozreview now.  Will all
the bugzilla links to those reviews go dead?  Or do we have to maintain a
second service in read-only mode forever?  This would not be a problem if
the patch/review information was also stored in bugzilla instead of solely
placed in a separate tool.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: W3C Charter Advance Notice: Web Platform (recharter) & Service Workers WGs

[Ben Kelly]

We have implementation close to review for one-shot sync.  I don't know of
any browser that has implemented and shipped periodic sync yet.

On Tue, Jul 11, 2017 at 2:49 PM, L. David Baron  wrote:

> On Tuesday 2017-07-11 11:38 -0700, L. David Baron wrote:
> > On Wednesday 2017-07-05 11:02 -0700, L. David Baron wrote:
> > > On Friday 2017-05-12 15:58 -0700, L. David Baron wrote:
> > > > The W3C gave advance notice that 2 new charters are under
> > > > development:
> > > >
> > > >   https://lists.w3.org/Archives/Public/public-new-work/
> 2017May/0006.html
> > > >   (which contains brief descriptions of what has changed)
> > > >
> > > >   Web Platform Working Group
> > > >   http://w3c.github.io/charter-html/webplat-wg.html
> > > >   https://github.com/w3c/charter-html/
> > > >
> > > >   Service Workers Working Group
> > > >   http://w3c.github.io/charter-drafts/sw-charter.html
> > > >   https://github.com/w3c/charter-drafts/
> > > >
> > > > Comments on these charters can be made in their respective github
> > > > repositories, or, if necessary, I can make comments that should be
> > > > made as statements "from Mozilla" on the Advisory Committee mailing
> > > > list.
> > >
> > > I realize I didn't repost when the official review started, but
> > > these charters are under a formal review whose deadline is today, as
> > > sent out in
> > > https://lists.w3.org/Archives/Public/public-new-work/2017Jun/0002.html
> > > https://lists.w3.org/Archives/Public/public-new-work/2017Jun/0003.html
> >
> > Also, for what it's worth, given offline feedback, I plan to support
> > the Service Workers WG charter.  Apparently much of the discussion
> > about service workers happens in WHATWG forums, but it still seems
> > valuable to have the work happening in W3C, and it seems to be
> > functioning reasonably.
>
> Though I think it may be worth looking a little bit more into
> Background Sync, which is a new addition to the Service Workers.
> Are we already involved in that work?  Is it something we support?
> (I'm particularly curious about periodicSync, which appears to add
> pretty substantial capability to run in the background, based on my
> reading of
> https://github.com/WICG/BackgroundSync/blob/master/explainer.md .)
>
> -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)
>
> ___
> 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: More Rust code

[Ben Kelly]

On Tue, Jul 11, 2017 at 4:57 AM, Nicholas Nethercote  wrote:

> On Tue, Jul 11, 2017 at 11:15 AM, Bobby Holley 
> wrote:
>
> > If I were the owner of that module I would consider implementing a policy
> >> something like the following:
> >>
> >> "When a person writes a new compiled-code component, or majorly rewrites
> >> an existing one, they should strongly consider writing it in Rust
> instead
> >> of C or C++. Such a decision will depend on the nature of the component.
> >> Components that are relatively standalone and have simple interfaces to
> >> other components are good candidates to be written in Rust, as are
> >> components such as parsers that process untrusted input."
> >>
> >
> > I think this is pretty uncontroversial. The high-level strategic decision
> > to bet on Rust has already been made, and the cost of depending on the
> > language is already sunk. Now that we're past that point, I haven't heard
> > anyone arguing why we shouldn't opt for memory safety when writing new
> > standalone code. If there are people out there who disagree and think
> they
> > have the arguments/clout/allies to make the case, please speak up.
> >
>
> As Gibson said: "The future is already here — it's just not very evenly
> distributed."
>
> The paragraph you wrote is obvious to you, but I suspect it's not obvious
> to everyone -- Mozilla has a lot of contributors. There may still be some
> Firefox developers who think that Rust something that other people do, and
> that isn't relevant to them or their component(s). There may be some
> Firefox developers who are interested in Rust, but feel intimidated or
> uncertain about using it. There may be some developers who are keen to use
> Rust, but haven't realized that we are past the experimental stage.
>

(Picking a somewhat random response to reply here...)

It would be really nice to have IPDL codegen support for rust.  I
considered using rust for my current task, but decided not to since I
would've had to build even more boilerplate to work with IPC c++ actors.  I
would've ended up with more glue code than actual functional code.

Another advantage of building rust IPDL codegen targets would be that we
could build service oriented code in the parent process in rust.  This
would be an incremental improvement that could offer additional safety in
the parent process without having to tackle webidle, cycle collection,
etc.  In particular, PBackground parent actors already cannot interface
with xpcom since they are OMT and would be a good candidate here.

Anyway, I think fixing these kinds of integration pain points would
accelerate our ability to use rust in gecko.  I would hesitate to make any
kind of mandates until these issues are addressed.

Thanks.

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

Re: Shipping Headless Firefox on Linux

[Ben Kelly]

On Thu, Jun 15, 2017 at 4:37 PM, Nathan Froyd  wrote:

> On Thu, Jun 15, 2017 at 2:02 PM, Brendan Dahl  wrote:
> > Headless will run less of the platform specific widget code and I don't
> > recommend using it for platform specific testing. It is targeted more at
> > web developers and testing regular content pages. There definitely will
> be
> > cases where regular pages will need to exercise code that would vary per
> > platform (such as fullscreen), but hopefully we can provide good enough
> > emulation in headless and work to have a consistent enough behavior
> across
> > platforms that it won't matter.
>
> Would it be feasible to use headless mode for mochitests (or reftests,
> etc. etc.)?  I know there are probably some mochitests which care
> about the cases you mention above (e.g. fullscreen), but if that could
> be taken care of in the headless code itself or we could annotate the
> tests somehow, it would be a huge boon for running mochitests locally,
> or even in parallel.  (We already have some support for running
> reftests/crashtests in parallel.)
>

There are some tests which fail if the "screen" is not a particular size.
Those might be a problem as well.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Shipping Headless Firefox on Linux

[Ben Kelly]

I typically use xvfb-run when executing tests locally.  How does headless
mode differ?  Does it just run faster, but at the cost not testing some
widget code?

On Wed, Jun 14, 2017 at 7:51 PM, Brendan Dahl  wrote:

> Hello All,
>
>
> As of Firefox 55 I intend to ship headless Linux support (Firefox without a
> GUI and X11 server connection). Headless mode is enabled via the --headless
> command line flag for Firefox and does not affect Firefox when running in
> normal mode or on Windows and macOS
>
>
>
> For those unfamiliar with the project, the main goal of headless browsing
> is to make web developer workflow and testing with Firefox easier. There
> are currently two ways to interact with headless Firefox by using either
> SlimerJS or Marionette (WebDriver/Selenium).
>
>
>
> Testing:
>
> The Marionette test suite is now run in headless mode alongside the normal
> mode as a tier 2 test on try. There are also some basic xpcshell tests that
> use a simple headless browser. If the Marionette tests remain reliable, I
> intend to bump them up to tier 1 in a few weeks [1].
>
>
>
> In the near future, I'll also be enabling headless mode on Windows and then
> will start work on macOS. We also are going to investigate some possible
> performance improvements for headless mode.
>
>
>
> If you run into any issues please file a bug in the new headless component
> [2].
>
>
>
> More info:
>
>
>
> Meta bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1338004
>
> Linux support: https://bugzilla.mozilla.org/show_bug.cgi?id=1356681
>
> Windows support: https://bugzilla.mozilla.org/show_bug.cgi?id=1355150
>
> MacOS support: https://bugzilla.mozilla.org/show_bug.cgi?id=1355147
>
> [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1373029
>
> [2]:
> https://bugzilla.mozilla.org/enter_bug.cgi?product=Firefox&;
> component=Headless
> .
>
>
>
> Previous blog post on SlimerJS support:
> https://adriftwith.me/coding/2017/04/21/headless-slimerjs-with-firefox/
> ___
> 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: JSBC: JavaScript Start-up Bytecode Cache

[Ben Kelly]

On Tue, Jun 13, 2017 at 5:50 AM, Nicolas B. Pierron <
nicolas.b.pier...@mozilla.com> wrote:

> The JavaScript Start-up Bytecode Cache⁰ is a project which aims at
> reducing the page load time by recording the bytecode generated during the
> last visits and by-pass the JavaScript parser.
>
> This project changes the way we process JavaScript script tags which are
> fetched from the network, and cached. After multiple visits¹, the bytecode
> would be encoded incrementally², as soon as the bytecode emitter generates
> it. Once we reached some idle time³, we save the content encoded
> incrementally as an alternate data on the cache⁴.  The cache contains a
> compressed version of the source, the bytecode of functions which got
> executed during the start-up of the page, and all non-executed functions
> encoded as source indexes⁵.
>
> On follow-up visits the script loader would load the alternate data
> instead⁶ of the source, and decode the bytecode either off-thread⁷ or on
> the current-thread.  This is expected to replace the syntax checker and the
> bytecode emitter for all recorded functions.
>

Just an FYI for people following along at home:

1. We don't support this when loading worker scripts yet.
2. If a page script load is intercepted by a service worker then this
optimization is effectively disabled.

There are a number of follow-up bugs filed to fix those things, but its a
non-trivial amount of work.

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

Re: Linux builds now default to -O2 instead of -Os

[Ben Kelly]

On Tue, Jun 6, 2017 at 3:07 PM, Chris Peterson 
wrote:

> On 6/6/17 10:33 AM, Boris Zbarsky wrote:
>
>> On 6/1/17 9:04 PM, Mike Hommey wrote:
>>
>>> Ah, forgot to mention that. No, it doesn't affect *our* shipped builds
>>> (because PGO uses a different set of optimization flags).
>>>
>>> But it does affect downstream builds that don't PGO.
>>>
>>
>> Based on the jump I see on June 2 at https://treeherder.mozilla.org
>> /perf.html#/graphs?timerange=2592000&series=%5Bmozilla-
>> central,80984697abf1f1ff2b058e2d9f0b351fd9d12ad9,1,1%5D&
>> series=%5Bmozilla-central,ae68c64ef8bfa104fded89971f1c2c6c90
>> 926dca,1,1%5D&series=%5Bmozilla-central,dd55da63ebce86ee3867
>> aa3b39975c2a90869ce2,1,1%5D it affects some of our talos tests too (the
>> ones running on non-pgo).
>>
>
> We stopped Talos testing of non-e10s builds on May 14, but it looks like
> we also stopped testing Linux PGO builds on May 15. Is that expected?
>

Why did we stop talos testing non-e10s?  Firefox for android is a tier 1
platform (right?) and uses non-e10s.  Do we have separate fennec talos
tests somewhere?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: more setTimeout() changes incoming

[Ben Kelly]

These setTimeout() changes have been pushed to inbound:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7c6116ac71e954805dccf3146d6fffcf28bbc0cf

Please need-info me if you encounter any problems.  I'll check back later.

Thanks!

Ben

On Tue, May 30, 2017 at 10:34 PM, Ben Kelly  wrote:

> FYI, this did not land last week.  However, I do intend to land it
> tomorrow pending a few last reviews.
>
> Also, I ended up making changes that keeps our setTimeout() firing
> behavior closer to our old behavior.  We can still fire slightly early as
> dictacted by our nsITimer implementation.  Our precision should be
> approximately the same as behavior.
>
> The main benefit of this landing will be reduced allocation and locking
> overhead when setTimeout()/setInterval() is used.
>
> On Thu, May 25, 2017 at 11:00 AM, Ben Kelly  wrote:
>
>> Hi all,
>>
>> I want to give everyone a heads-up that I'm hoping to push some
>> setTimeout() changes to inbound in the next day or two:
>>
>> https://bugzilla.mozilla.org/show_bug.cgi?id=1363829
>>
>> The main result people should be aware of is that setTimeout() will be
>> more accurate and precise after this lands [1]:
>>
>> 1. We will no longer be able to fire setTimeout() handlers early.
>> 2. Assuming an idle main thread setTimeout() handlers will be less
>> delayed.
>> 3. setTimeout(f, 0) may be a simple runnable dispatch now and can fire a
>> bit faster than before.
>>
>> As you can imagine, changing setTimeout() timings has interesting effects
>> on our test results.  I've been working hard to find and fix intermittents,
>> but there will undoubtedly be some changes to our orange after landing this.
>>
>> If you have an intermittent you think appeared after this lands, please
>> look for any possible timing races in the test.  Also, feel free to NI me
>> to help investigate.
>>
>> Again, I've worked hard to squash the intermittents I've seen in try, so
>> I don't believe there will be any super frequent failures from this bug.
>> (Or I wouldn't be landing, etc...)
>>
>> Please let me know if there are any questions or concerns.  Thanks!
>>
>> Ben
>>
>> [1] For example, using this tool:
>>
>> https://people-mozilla.org/~bkelly/timeout-precision/?mode=
>> delta&delay=10&count=100
>>
>> My current FF55 on windows 10 gives the following stats for "differences
>> from specified firing time" in milliseconds:
>>
>> min:-0.120 mean:1.922 median:1.830 max:5.180 stdev:1.753
>>
>> So sometimes it fires as much as 120us early and has a median firing time
>> 1.8ms late.  With bug 1363829 we get:
>>
>> min:0.010 mean:0.403 median:0.025 max:4.155 stdev:1.098
>>
>> So we don't fire early any more and our median firing time is only 25us
>> late.
>>
>
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: more setTimeout() changes incoming

[Ben Kelly]

FYI, this did not land last week.  However, I do intend to land it tomorrow
pending a few last reviews.

Also, I ended up making changes that keeps our setTimeout() firing behavior
closer to our old behavior.  We can still fire slightly early as dictacted
by our nsITimer implementation.  Our precision should be approximately the
same as behavior.

The main benefit of this landing will be reduced allocation and locking
overhead when setTimeout()/setInterval() is used.

On Thu, May 25, 2017 at 11:00 AM, Ben Kelly  wrote:

> Hi all,
>
> I want to give everyone a heads-up that I'm hoping to push some
> setTimeout() changes to inbound in the next day or two:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1363829
>
> The main result people should be aware of is that setTimeout() will be
> more accurate and precise after this lands [1]:
>
> 1. We will no longer be able to fire setTimeout() handlers early.
> 2. Assuming an idle main thread setTimeout() handlers will be less delayed.
> 3. setTimeout(f, 0) may be a simple runnable dispatch now and can fire a
> bit faster than before.
>
> As you can imagine, changing setTimeout() timings has interesting effects
> on our test results.  I've been working hard to find and fix intermittents,
> but there will undoubtedly be some changes to our orange after landing this.
>
> If you have an intermittent you think appeared after this lands, please
> look for any possible timing races in the test.  Also, feel free to NI me
> to help investigate.
>
> Again, I've worked hard to squash the intermittents I've seen in try, so I
> don't believe there will be any super frequent failures from this bug.  (Or
> I wouldn't be landing, etc...)
>
> Please let me know if there are any questions or concerns.  Thanks!
>
> Ben
>
> [1] For example, using this tool:
>
> https://people-mozilla.org/~bkelly/timeout-precision/?
> mode=delta&delay=10&count=100
>
> My current FF55 on windows 10 gives the following stats for "differences
> from specified firing time" in milliseconds:
>
> min:-0.120 mean:1.922 median:1.830 max:5.180 stdev:1.753
>
> So sometimes it fires as much as 120us early and has a median firing time
> 1.8ms late.  With bug 1363829 we get:
>
> min:0.010 mean:0.403 median:0.025 max:4.155 stdev:1.098
>
> So we don't fire early any more and our median firing time is only 25us
> late.
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

more setTimeout() changes incoming

[Ben Kelly]

Hi all,

I want to give everyone a heads-up that I'm hoping to push some
setTimeout() changes to inbound in the next day or two:

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

The main result people should be aware of is that setTimeout() will be more
accurate and precise after this lands [1]:

1. We will no longer be able to fire setTimeout() handlers early.
2. Assuming an idle main thread setTimeout() handlers will be less delayed.
3. setTimeout(f, 0) may be a simple runnable dispatch now and can fire a
bit faster than before.

As you can imagine, changing setTimeout() timings has interesting effects
on our test results.  I've been working hard to find and fix intermittents,
but there will undoubtedly be some changes to our orange after landing this.

If you have an intermittent you think appeared after this lands, please
look for any possible timing races in the test.  Also, feel free to NI me
to help investigate.

Again, I've worked hard to squash the intermittents I've seen in try, so I
don't believe there will be any super frequent failures from this bug.  (Or
I wouldn't be landing, etc...)

Please let me know if there are any questions or concerns.  Thanks!

Ben

[1] For example, using this tool:

https://people-mozilla.org/~bkelly/timeout-precision/?mode=delta&delay=10&count=100

My current FF55 on windows 10 gives the following stats for "differences
from specified firing time" in milliseconds:

min:-0.120 mean:1.922 median:1.830 max:5.180 stdev:1.753

So sometimes it fires as much as 120us early and has a median firing time
1.8ms late.  With bug 1363829 we get:

min:0.010 mean:0.403 median:0.025 max:4.155 stdev:1.098

So we don't fire early any more and our median firing time is only 25us
late.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: NetworkInformation

[Ben Kelly]

Can the people who have concerns about the NetworkInformation API please
provide the feedback to google on this blink-dev thread:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/UVfNMH50aaQ/CXY6S39TBQAJ

In particular, I think they tried to consider privacy in this part of the
spec:

https://wicg.github.io/netinfo/#privacy-considerations

Thanks.

Ben

On Fri, Dec 23, 2016 at 10:43 AM, Eric Rescorla  wrote:

> On Thu, Dec 22, 2016 at 10:31 PM,  wrote:
>
> > On Wednesday, December 21, 2016 at 12:51:10 AM UTC+11, Eric Rescorla
> wrote:
> > > I'm not really following this argument. Usually when a document has
> been
> > > floating
> > > around a long time but clearly has basic design issues and can't get
> > > consensus,
> > > even when a major vendor has implemented it, that's a sign that it
> > > *shouldn't*
> > > be standardized until those issues are resolved. That's not standards
> > > fatigue,
> > > it's the process working as designed.
> >
> > The API addresses the use cases, but people here see those use cases as
> > too basic because they don't represent average users (e.g., Boris'
> somewhat
> > esoteric network setup). Most people have wifi at home, which is somewhat
> > unmetered - and access to mobile data, which often costs more (but not
> > always true).
> >
> > The API, though ".type", allows the user and app to have a conversation
> > about that: "you want me to download stuff over mobile? Its might cost
> ya,
> > but if you are ok with it...".
> >
>
> I don't really think this addresses my argument, which is not about any of
> the technical details, but is rather about whether we should adopt
> something that's clearly not very good -- which it seems clear you are
> conceding here -- just because it's been floating around a long time and
> people are tired.
>
> -Ekr
> ___
> 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: Avoiding jank in async functions/promises?

[Ben Kelly]

On Wed, May 17, 2017 at 10:19 PM, Ben Kelly  wrote:

> FWIW, we have a similar problem in the native TimeoutManager::RunTImeout()
> method.  I'm using a time budget approach to make it adapt to different
> hardware better.
>

I meant to include the bug number here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1343912
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Avoiding jank in async functions/promises?

[Ben Kelly]

On Wed, May 17, 2017 at 10:03 PM, Boris Zbarsky  wrote:

> On 5/17/17 9:22 PM, Mark Hammond wrote:
>
>> I'm wondering if there are any ideas about how to solve this optimally?
>>
>
> I assume https://w3c.github.io/requestidlecallback/#the-requestidleca
> llback-method doesn't have quite the right semantics here?  That would
> let you run when the browser is idle, and give you some idea of how long
> you can run for before you should yield.
>

If rIC is not the right semantics, we could also set a time budget instead
of a magic flat limit.  Every N operations call performance.now() and check
to see if the configured time exceeds the limit.

FWIW, we have a similar problem in the native TimeoutManager::RunTImeout()
method.  I'm using a time budget approach to make it adapt to different
hardware better.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Ambient Light Sensor API

[Ben Kelly]

The post suggests that limiting precision would mitigate the issue.  We
could do that immediately while we wait for telemetry to roll in.

The post says reducing the frequency of the readings would not be very
effective, but maybe we should reduce the frequency anyway?  Possibly
firing an event every 100ms to 200ms to report on light conditions seems
rather bad for perf/battery.

On Mon, Apr 24, 2017 at 9:24 AM, Frederik Braun  wrote:

> Hi,
>
> there is a relatively recent blog post [1] by Lukasz Olejnik and Artur
> Janc that explains how one can steal sensitive data using the Ambient
> Light Sensor API [2].
>
> We ship API and its enabled by default [3,4] and it seems we have no
> telemetry for this feature.
>
>
> Unshipping for non-secure context and making it HTTPS-only wouldn't
> address the attack.
>
> The API as implemented is using the 'devicelight' event on window.
> I suppose one might also be able to implement a prompt for this, but
> that doesn't sound very appealing (prompt fatigue, etc., etc.).
>
>
> What do people think we should do about this?
>
>
>
> Cheers,
> Freddy
>
>
>
>
>
> [1]
> https://blog.lukaszolejnik.com/stealing-sensitive-
> browser-data-with-the-w3c-ambient-light-sensor-api/
> [2] https://www.w3.org/TR/ambient-light/
> [3] It is behind the dom.sensors.enabled (sic!) flag.
> [4]
> http://searchfox.org/mozilla-central/source/dom/system/nsDeviceSensors.cpp
> ___
> 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: A reminder about commit messages: they should be useful

[Ben Kelly]

On Mon, Apr 17, 2017 at 9:21 PM, Nicholas Nethercote  wrote:

> > That is why we have links to the bug. Bug should always be the unite of
> > truth telling
> > why some change was done. Bugs tend to have so much more context about
> the
> > change than any individual commit message can or should have.
>
> With all due respect, I think you have a different view on this to at least
> some people (and perhaps most people).
>

FWIW I agree with Olli.  I look for a good one line summary of the change,
but beyond that I find you really do need to look at the bug to get the
full context.

I don't object to people writing longer commit messages, but that
information needs to be in the bug.  Our tools today (splinter and
mozreview) don't do that automatically AFAIK.  I think there is an hg
extension you can get to do it with splinter.

I find it much more useful to have an explanatory comment in the bug since
I can see it in context with everything else, links, etc.  Its also clear
what version of the patch its associated with.  Commit messages often are
not updated with changes to the patch and can be slightly wrong.

Finally, as has been mentioned in previous threads, any real nuance should
be in the patch itself as comments.

Both bugs and code comments are searchable and more easily discoverable.
The tools for finding information in blame are much harder to use.  I
greatly prefer bug and code comments.

Just my two cents.  (I wasn't going to chime in here, but felt compelled
since Olli's view was being cast as rare.)

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

Re: Better download security through browsers

[Ben Kelly]

We now have SRI and support integrity attributes on elements like

Re: e10s-multi update and tests

[Ben Kelly]

On Wed, Mar 22, 2017 at 9:12 PM, Andrew McCreight 
wrote:

> On Wed, Mar 22, 2017 at 6:00 PM, Nicholas Nethercote <
> n.netherc...@gmail.com
> > wrote:
>
> > Do we have a clear definition of "content process"? I.e. does/will it
> > include:
> >
> > - GMP processes (no?)
> > - GPU process (probably not?)
> > - file:// URL processes (probably should?)
> > - Web Extensions processes (probably should?)
> > - ServiceWorker processes (probably should?)
> >
>
> The most operational definition is that a content process is a process
> where XRE_IsContentProcess() returns true. From the enum GeckoProcessType,
> you can see the various other possibilities. So the answers to your
> questions are then probably that GMP and GPU are not, but the others are.
> Though maybe you are asking which processes count against the limit of 4.
>

FWIW, the intention is that the worker process would not count against the
limit.  It would not contain any windows and would be significantly smaller
in size.

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

Re: unowned module: Firefox::New Tab Page, help me find an owner

[Ben Kelly]

On Wed, Mar 22, 2017 at 10:00 AM, David Burns  wrote:

> On 22 March 2017 at 13:49, Ben Kelly  wrote:
>
>> Finding someone to own the feature and investigate intermittents is
>> important too, but that doesn't mean the tests have zero value.
>>
>
> This just strikes me that we are going to disable until they are all gone
> then we have dead code in the tree and still no one to own it. Its a longer
> process that could end up at the same end point.
>

Are *all* the tests intermittent?  My quick read of the bug was that its a
single test.  I just don't understand the logic of *removing green tests*
from the tree.  The idea they might one day become intermittent is not
compelling to me.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: unowned module: Firefox::New Tab Page, help me find an owner

[Ben Kelly]

On Wed, Mar 22, 2017 at 9:39 AM,  wrote:

> On Wednesday, March 22, 2017 at 9:35:35 AM UTC-4, Ben Kelly wrote:
> > You plan to delete all the tests?  This seems somewhat extreme for a
> > shipped feature.  Why not disable just the tests that are intermittent?
>
> I agree that does sound extreme, but if nobody cares about the tests or
> will accept responsibility if we have failures, then they have no real
> value.  I assume they have value and this is why I am asking for help to
> find someone who cares about the code and will take responsibility for the
> feature and related tests.
>

I don't agree they have no value "if nobody cares about the tests or will
accept responsibility if we have failures".  If someone changes something
in platform and these tests turn perma-red, that person has to fix their
code.  This is how we prevent regressions.

Finding someone to own the feature and investigate intermittents is
important too, but that doesn't mean the tests have zero value.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: unowned module: Firefox::New Tab Page, help me find an owner

[Ben Kelly]

On Wed, Mar 22, 2017 at 9:22 AM,  wrote:

> I have not been able to find an owner for the Firefox::New Tab Page
> bugzilla component (bug 1346908).  There are 35 tests in the tree and
> without anyone to assume responsibility for them when they are intermittent
> (bug 1338848), I plan to delete them all if I cannot get an owner by the
> end of the month including someone who will sign up to be the triage owner
> for the bugzilla component.
>

You plan to delete all the tests?  This seems somewhat extreme for a
shipped feature.  Why not disable just the tests that are intermittent?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: windows build anti-virus exclusion list?

[Ben Kelly]

On Fri, Mar 17, 2017 at 3:40 PM, Ted Mielczarek  wrote:

> Yeah, the JS engine uses a lot more complex C++ features than the rest of
> the code in our tree, so it takes longer to compile. This is also why the
> `FILES_PER_UNIFIED_FILE` setting is lower in js/src than the rest of the
> tree. We do try to build js/src pretty early in the build, although the
> exact workings of the compile tier is not something I currently understand.
> One thing we could try here would be to hack up some instrumentation to
> record the time taken to compile each source file, which would let us
> determine if we need to tweak `FILES_PER_UNIFIED_FILE` lower, at least.
>

Hmm.  The "we do try to build js/src pretty early in the build" statement
doesn't seem to match what I am seeing.  Its one of the last things to
build on my machine.  Also, it seems to be serialized with building other
libraries.

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

Re: windows build anti-virus exclusion list?

[Ben Kelly]

On Fri, Mar 17, 2017 at 2:52 PM, Ben Kelly  wrote:

> On Fri, Mar 17, 2017 at 2:43 PM, Ted Mielczarek 
> wrote:
>
> Yeah, I specifically meant "CPU-bound during the compile tier", where we
>> compile all the C++ code. If you look at the resource usage graphs I
>> posted it's pretty apparent where that is (the full `mach
>> resource-usage` HTML page has a nicer breakdown of tiers). The stuff
>> before and after compile is not as good, and the tail end of compile
>> gets hung up on some long-pole files, but otherwise it does a pretty
>> good job of saturating available CPU. I also manually monitored disk and
>> memory usage during the second build, and didn't see much there. The
>> disk usage showed ~5% active time, presumably mostly the compiler
>> generating output, and memory usage seemed to be stable at around 9GB
>> for most of the build (I didn't watch during libxul linking, I wouldn't
>> be surprised if it spikes then).
>>
>
> That "long pole file" at the end of the js lib is over 10% of my compile
> time.  That's not very good parallelism in the compile stage IMO.
>

This is the part of the build I'm talking about:

15:17.80 Unified_cpp_js_src8.cpp
15:17.90 Unified_cpp_js_src38.cpp
15:18.33 Unified_cpp_js_src40.cpp
15:19.96 Unified_cpp_js_src41.cpp
15:21.41 Unified_cpp_js_src9.cpp
16:59.13 Interpreter.cpp
16:59.15 js_static.lib
16:59.99 module.res
17:00.04 Creating Resource file: module.res
17:00.81 StaticXULComponentsStart.cpp
17:00.99 nsDllMain.cpp

For the 1:38 between Unified_cpp_js_src9.cpp and Interpreter.cpp only a
single cl.exe process is running.  I guess thats closer to 8% of the total
build time.  Still seems very weird to me.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: windows build anti-virus exclusion list?

[Ben Kelly]

On Fri, Mar 17, 2017 at 2:43 PM, Ted Mielczarek  wrote:

> > The 14min measurement must have been for a partial build.  With defender
> > disabled the best I can get is 18min.  This is on one of the new lenovo
> > p710 machines with 16 xeon cores.
>
> Nope, full clobber builds: `./mach clobber; time ./mach build`. (I have
> the same machine, FWIW.) The svg files I uploaded were from `mach
>

Sorry.  I misread.  I thought you were referring to my earlier email about
dropping from 20+ minutes to 14 minutes.  My 14 minute measurement was
erroneous.  I seem to get 18 minute clobber builds.


> Yeah, I specifically meant "CPU-bound during the compile tier", where we
> compile all the C++ code. If you look at the resource usage graphs I
> posted it's pretty apparent where that is (the full `mach
> resource-usage` HTML page has a nicer breakdown of tiers). The stuff
> before and after compile is not as good, and the tail end of compile
> gets hung up on some long-pole files, but otherwise it does a pretty
> good job of saturating available CPU. I also manually monitored disk and
> memory usage during the second build, and didn't see much there. The
> disk usage showed ~5% active time, presumably mostly the compiler
> generating output, and memory usage seemed to be stable at around 9GB
> for most of the build (I didn't watch during libxul linking, I wouldn't
> be surprised if it spikes then).
>

That "long pole file" at the end of the js lib is over 10% of my compile
time.  That's not very good parallelism in the compile stage IMO.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform