Re: refcounting - which types to use ?

[Aryeh Gregor]

On Thu, Aug 17, 2017 at 7:07 AM, Enrico Weigelt, metux IT consult
 wrote:
> OTOH, is there a way to create versions that really return it as rval,
> so I conveniently could just call like that ?
>
>nsCOMPtr list = addrbook->FindRecipients(addr);

You can do this with a return type of
already_AddRefed.  Then instead of

  list.forget(_retval);
  return NS_OK;

you do just

  return list.forget();

Note that in this case you cannot ignore the return value -- it must
be assigned to an nsCOMPtr or a RefPtr, or else it will leak.  Ideally
we would allow a return type of nsCOMPtr&&, but
currently I think we don't.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Linking with lld instead of ld/gold

[Aryeh Gregor]

On Mon, Aug 14, 2017 at 12:08 AM, Sylvestre Ledru  wrote:
> Packages for lld on Debian & Ubuntu are available on https://apt.llvm.org/

Ubuntu 16.04 and later also has an lld-4.0 package in its own repos
(sudo apt install lld-4.0).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to change editor newline behavior

[Aryeh Gregor]

On Thu, Apr 6, 2017 at 3:34 PM, Ehsan Akhgari  wrote:
> Thanks for the follow-ups and for filing bug 1354060.  But now that we
> know that this affected Gmail, I'll note that the risk around this is
> still there, and while keeping this on the pre-release channels for a
> while reduces the risk somewhat, we should still be super careful when
> we decide to let it go to release, and preferably do so in coordination
> with the Web Compat team and in preparation to deal with any fallout.  :-)

I agree that it makes sense to not let this ride the trains at the
normal pace.  I guess it's Masayuki's decision how exactly to proceed.
(At any rate, it's certainly not mine, thankfully.  :) )
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to change editor newline behavior

[Aryeh Gregor]

On Wed, Apr 5, 2017 at 11:22 PM, Daniel Veditz  wrote:
> One line or a thousand isn't the point; building, release and update
> testing, and shipping 90 locales times 6 platforms is a huge amount work. If
> we have a pref and break something we can back it out easily and quickly. We
> could even do a slow roll-out independent of a release if we're extra
> concerned about compat (as we have done with e10s and sha1 deprecation, for
> example).

Then it certainly sounds good to make this a pref.  I filed bug 1354060.

Incidentally, bug 1353913 proves that Gmail does use our line-breaking
behavior, and so presumably so do other editors.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Enabling Pointer Events in Firefox (desktop) Nightly on Mac and Linux

[Aryeh Gregor]

On Wed, Apr 5, 2017 at 7:34 PM, Tom Ritter  wrote:
> It looks like this exposes pointerType, which reveals whether the user
> is using a mouse, pen, or touch input.
>
> It also exposes detailed information about the geometry of the input
> (size of the thing pointing, pressure, tilt, twist.
>
> All of these are more detailed information that websites currently
> receiving, meaning that this can be used as a mechanism for
> fingerprinting (and tracking) users.

I think this has been discussed here before, but I don't recall a firm
conclusion: has anyone established whether a nontrivial number of
users are non-fingerprintable as things stand?  If the vast majority
of users can be fingerprinted right now, and there are no realistic
plans to change that, it doesn't seem like we should care about
increasing fingerprinting ability.  I haven't investigated, but I'd be
surprised if there are a lot of users who can't be fingerprinted yet,
given the huge and rapidly-expanding number of features in the web
platform.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to change editor newline behavior

[Aryeh Gregor]

On Wed, Apr 5, 2017 at 4:48 PM, Ehsan Akhgari  wrote:
> Originally it seemed that you are working under the assumption that most
> sites are overriding our default newline handling behavior.  This is
> very easy to measure through telemetry by adding a probe for example
> that looks when an HTML editor object is destroyed whether the
> defaultParagraphSeparator command was used to override our default
> behavior, you can send a value of 0 for the false case and a value of 1
> for the true case and we can get a percentage of pages where this
> override actually happens on based on this data.

I think I wasn't clear.  By "overriding" I meant that when the user
presses Enter, they intercept the keystroke and modify the DOM
themselves, and the editor code is never invoked.  I didn't mean that
the sites alter defaultParagraphSeparator.  So we could add a probe
that detects when the editor's Enter-handling code is run at all.

>> If it's not so low, however, we'd need
>> to look at the actual sites to see if they break.  Can we do that
>> somehow through telemetry, or is it a privacy problem?
>
> Detecting the actual breakage that can happen is much more difficult in
> code because you would need to first define what the breakage would
> result in to try to detect it in code which is extremely difficult in
> this case.

If we had a list of top sites that used our Enter-handling code, we
could test them manually.  (But such testing would be unlikely to be
comprehensive.)

> Getting the kind of data I'm suggesting above _now_ that the patch is
> landed seems rather pointless.  There seems to be a lot of disagreement
> on the degree of the risk involved in this change in the first place,
> and until we agree on the level of risk arguing about the details like
> this may be pointless.

It's not pointless if we pref it only to Aurora, and leave it there
until we have more data.

> At the end of the day, this is Masayuki's call.  I certainly understand
> that in the past with changes like this we've had a lot of difficulty
> detecting regressions before the patches have hit the release channel,
> so I'm not sure how much keeping the patch on pre-release channels would
> really help.  :-(  But to me it seems like the kind of thing that we'd
> want to be able to quickly turn off on the release channel through
> shipping a hotfix add-on that sets a pref if something goes wrong...

FWIW, changing the default back to  is a one-line change already.

> Perhaps I'm missing something about the nature of what changed here.
> How is this seldom used?  Am I misunderstanding that the change happened
> was how *Gecko* reacts to the user pressing Enter by default in a
> contenteditable section?  It's true that some editing widgets override
> this behavior somehow, but I'd be really shocked if that's true 100% of
> the time, so I don't understand how you argue this is a seldom used
> feature...

Yes, I do mean that AFAIK high-profile editing frameworks do override
this kind of behavior.  Johannes Wilm seems to be the founder of the
"Fidus Writer" editor project, and when I asked "Do editors all
override hitting Return, Backspace, etc.?" he said "Those involve
taking apart and merging paragraphs, so I cannot really imagine how
one would create an editor without doing that. At the very least one
will need to monitor what the browser does and then adjust if needed."
   The
second sentence is what we have to worry about, though.

One thing I know for sure is, when I looked into it several years ago,
execCommand() usage among high-profile editors seemed to be
nonexistent.  Once you're writing all the code yourself to handle
styling and block formatting and so on, overriding the behavior for
hitting Enter is a no-brainer.

It wouldn't be hard to test whether various sites use our built-in
linebreak behavior when the user hits Enter.  Just put a printf in the
relevant function and see if it gets hit.  Maybe I'll do that.

> I guess I'm personally coming from the perspective of having bitten by
> regressing various things about the editor behavior too many times in
> the past, and every time it happened, we could make an argument about
> how it's extremely unlikely to happen beforehand.  :-)

Yes, on reconsideration, I expect some fallout.  Phasing this in more
cautiously seems like a good idea to me.  I think it might be best to
just leave it in Aurora and Beta for longer than usual before
promoting it.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to change editor newline behavior

[Aryeh Gregor]

On Wed, Apr 5, 2017 at 3:12 AM, Ehsan Akhgari  wrote:
> Exactly.  We can hypothesize either way, but we certainly can't know easily
> without getting some data first.  But unfortunately it's not possible to
> collect data about what sites are doing in terms of DOM fix-ups like this.
> We can, at least, collect data about whether they are overriding the newline
> behavior wholesale though.  Is there any reason why we should not at least
> first collect this data before changing the behavior here?

What exact data would you want?  We could collect data on how often
our built-in newline behavior is used, and if it's low enough we'd be
confident the change is safe.  If it's not so low, however, we'd need
to look at the actual sites to see if they break.  Can we do that
somehow through telemetry, or is it a privacy problem?

If someone has suggestions for the exact telemetry probe to use here,
I'm happy to add one, and maybe keep the change in Aurora until we get
data to make a decision.  I'm not familiar enough with telemetry
(either the theoretical options or our implementation) to decide what
the right probe is.

On Wed, Apr 5, 2017 at 3:31 AM, Benjamin Smedberg  wrote:
> I agree that it doesn't seem likely that telemetry can answer this sort of
> question. However, we could collect data! Pick N top editing tools and
> actually test their behavior. We probably can't get full confidence, but we
> can get a much better picture of the risk involved. If we break (or
> significantly change behavior) on five sites out of 40, that's a strong
> indicator that we're going to have problems.
>
> As an example, have we already tested or is it in a plan to test:
> google docs
> basic and rich text editors on wikipedia
> office 365
> github editor
> common rich text editor libraries, and common CRM software (I don't have a
> list)
> the top hosted email sites: gmail, yahoo, hotmail/outlook, aol, icloud,
> yandex
>
> Being able to assert, before landing this change, that it doesn't break any
> of these sites, would really help in terms of making assertions about the
> risk profile.

I did not test this specific change on those sites.  However, some
years ago I did research execCommand() usage on the web, and found
that high-profile richtext editors essentially didn't use it, because
of browser incompatibilities.  Instead, they wrote their own
functions.  The same incompatibilities exist in newline behavior, so
it's reasonable to say that they would write their own functions for
that too.

This is also supported by a discussion I had with a couple of
developers of major richtext editing libraries (I don't remember which
offhand).  They said that changes like this make no difference to
them, because they don't use the functionality anyway.  They're
interested in fixes in things like selection handling, which they do
use, and features that allow them to more easily override browser
behavior.

Also anecdotally, in terms of dealing with editor bugs like this --
the reports most often come from Thunderbird.  Maybe Ehsan or Masayuki
could weigh in too, but I think that we get very few bug reports in
these parts of the editor from real-world websites.  (We do get some
reports in other parts of the editor, like selection/focus handling.)
This also suggests websites aren't actually using this code much.
(Although maybe it means they just work around our bugs already.)

In fact, I dropped this patch set for a while because the feature is
seldom-used enough on the web that it doesn't seem worth fixing.  I
get the impression that Ehsan shares this point of view.

It's also worth noting that contenteditable is a very complicated
feature to use in real life, particularly given browser
incompatibilities, and I think almost all sites that use it are either
very large or use one of the major rich-text editing libraries.  If
I'm correct, then we don't have to worry so much about breaking a long
tail of small sites that won't get reported quickly.  If Gmail,
Wikipedia, TinyMCE, etc. break, we (or they) are likely to get reports
soon enough.  Large sites are also usually well-maintained and do
their own testing, and will fix the issue quickly.  (But if a library
breaks, or software like MediaWiki, there will be a long tail of old
sites that will remain broken even after the library is fixed, because
they keep using old versions of the library.)

So that's my reasoning for why I don't think this is *so* risky.  But
I agree that I don't really know.  As I said, I'd be happy to let this
stay on Aurora or beta for longer, and/or add some telemetry (if
someone tells me what telemetry we want).
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to change editor newline behavior

[Aryeh Gregor]

On Tue, Apr 4, 2017 at 5:57 PM, Ehsan Akhgari  wrote:
> I don't own this module any more, so this isn't my call to make, but if
> I had to choose what to do here, I would probably either choose to not
> change our behavior (since I'm not sure what we're gaining here
> concretely -- as AFAIK we're not investing in bringing our behavior on
> par with other engines on a more broad basis with regards to editing),

Masayuki seems to be in favor of trying to match Blink more.

> or at the lack of that, adding some telemetry first to get data on how
> often the defaultParagraphSeparator command is used in the wild, since
> AFAICT your change is basically only Web compatible on the assumption
> that this command is used quite heavily.

I doubt it's used much.  My assumption is only that not many sites are
UA-sniffing Firefox, finding the s, and modifying them in some way
that breaks if they're no longer s.  That could still be totally
wrong, though!

> On the idea of the test plan that Benjamin brought up, I'm not sure what
> to put in such a test plan, due to the issue I mentioned above (it being
> totally non-obvious what the expected breakage of this change would look
> like.)

We could put the default defaultParagraphSeparator change behind a
pref and leave the pref off on release (or on beta and release?) for
some period and see if we get bug reports.  I don't think there's any
way to detect breakage by telemetry, so we'd have to rely on user
reports.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to change editor newline behavior

[Aryeh Gregor]

On Mon, Apr 3, 2017 at 11:39 PM, Benjamin Smedberg
 wrote:
>
> I'd like to encourage you to set up a test plan for this. My impression of 
> the risk profile of this work is that we could easily break some really 
> important use-cases, and it's likely that sites customize for gecko behavior 
> and rely on it, either accidentally or on purpose. This is definitely the 
> kind of thing that would be worth rolling out carefully and perhaps slowly. 
> Will this behavior be behind a pref which is easy to flip, test, and roll out?

As implemented, it is not behind a pref.  Masayuki didn't request a
gradual rollout, so I pushed to inbound already.  I suspect it will
not actually break anything, because sites that use the editor
normally avoid browser compatibility issues here by completely
overriding the newline behavior, so they will be unaffected.  If any
sites actually do break from this, I would be very interested to have
the data that people are really using our default newline behavior!

I'd like to know what Ehsan and Masayuki think about the likely compat
impact here.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Intent to change editor newline behavior

[Aryeh Gregor]

In our rich-text editor (used in Firefox for designMode and
contenteditable), when the user hits Enter, we have historically always
inserted a .  This does not match any other browsers, which use  or
 as line separators.  In bug 1297414, I'm changing our behavior to use
 as a line separator.  This matches Blink/WebKit.

So if you have the text "foobar" and hit Enter in between the "foo" and the
"bar", previously you would get "foobar", and soon you will get
"foobar".  The defaultParagraphSeparator command can
be used to change the separator to "p" instead (which matches Edge's
default behavior last I checked).

Pages or embedders that want to keep the old behavior can run the following
command: document.execCommand("defaultParagraphSeparator", false, "br").

This change is not likely to affect high-profile sites that use rich-text
editing (webmail etc.), because due to browser incompatibility, these sites
all override this behavior anyway.

Our new behavior is as specified in the essentially unmaintained editing
specification that I wrote several years ago, and tested by the
web-platform-tests editing suite.  (Except that the "br" value to
defaultParagraphSeparator is unspecified, and is a Mozilla-specific
extension for backwards compatibility.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Removing navigator.buildID?

[Aryeh Gregor]

On Mon, Oct 31, 2016 at 4:00 PM, Henri Sivonen  wrote:
> On Mon, Oct 31, 2016 at 3:01 PM, Aryeh Gregor  wrote:
>> If the concern is fingerprinting, perhaps it could be exposed only to
>> sites that the user is logged into (assuming we have a good working
>> definition of "logged in")?
>
> I think that's over-engineering it.
>
> I suggest we make it behave the current way for chrome,
> https://*.mozilla.org and https://*.netflix.com origins and make it
> return a constant otherwise.

This only helps us and Netflix.  Other sites that want to track fixes
will be left out in the cold.

Taking a step back: is fingerprinting really a solvable problem in
practice?  At this point, are there a significant fraction of users
who can't be fingerprinted pretty reliably?  Inevitably, the more
features we add to the platform, the more fingerprinting surface we'll
expose.  At a certain point, we might be taking away a lot of features
for the sake of trying to stop the unstoppable.  (I have no idea if
this is actually true now, though.  This is a genuine question.  :) )
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Removing navigator.buildID?

[Aryeh Gregor]

On Mon, Oct 31, 2016 at 10:02 AM, Chris Peterson  wrote:
> Alternately, buildID could be hidden behind a domain whitelist pref, seeded
> with sites working with Mozilla. If you are, say, a Netflix customer, they
> already know who you are, so exposing your buildID to them is not much of a
> tracking risk. :)

If the concern is fingerprinting, perhaps it could be exposed only to
sites that the user is logged into (assuming we have a good working
definition of "logged in")?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to restrict to secure contexts: navigator.geolocation

[Aryeh Gregor]

On Tue, Oct 25, 2016 at 9:43 PM, Eric Rescorla  wrote:
> Setting aside the policy question, the location API for mobile devices
> generally
> gives a much more precise estimate of your location than can be obtained
> from the upstream network provider. For instance, consider the case of the
> ISP upstream from Mozilla's office in Mountain view: they can only localize
> a user to within 50 meters or so of the office, whereas GPS is accurate to
> a few meters.

Yes, but the privacy impact of such an increase in precision is much
less than if you only knew geo IP data.

> And of course someone who is upstream from the ISP may just
> have standard geo IP data.

Are there real-world MITMs who are upstream from the ISP?
(Governments presumably compromise the ISP itself by court order or
equivalent.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to restrict to secure contexts: navigator.geolocation

[Aryeh Gregor]

On Tue, Oct 25, 2016 at 8:12 PM, Anne van Kesteren  wrote:
> The basic problem is prompting the user at all for non-HTTPS since
> that leads them to think they can make an informed decision whereas
> that's very much unclear. So prompting more would just make the
> problem worse.
>
> We want to get to a place where when we prompt the user on behalf of a
> website we have some certainty who is asking the question (i.e.,
> HTTPS).

By that logic, we should not permit users to submit forms to non-HTTPS
either.  I agree that if we were designing the web from scratch we
would absolutely require HTTPS for everything, but in reality we have
to make a cost-benefit analysis in each case.  That means analyzing
the threats to our users' privacy or security and deciding whether it
outweighs the user annoyance.  If the prospect of a privacy leak is
implausible or not a big privacy compromise, it doesn't necessarily
outweigh the cost of aggravating users.  I don't think that privacy or
security issues are exempt from cost-benefit analysis like any other
feature or bug fix -- they're unusually important, but still do not
have infinite value.

In this specific case, it seems that the usual candidates for MITMing
(compromised Wi-Fi, malicious ISP) will already know the user's
approximate location, because they're the ones who set up the Internet
connection, and Wi-Fi has limited range.  What exactly is the scenario
we're worried about here?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to implement and ship: SVGElement.prototype.dataset

[Aryeh Gregor]

On Thu, Aug 11, 2016 at 12:53 AM, Boris Zbarsky  wrote:
> Summary: HTML elements have a .dataset property that allows convenient
> somewhat structured access to data-* attributes.  The proposal is to add
> this to SVG elements too, following corresponding changes in the SVG
> specification.

Why doesn't this just go on Element, like .className and .id?  Is
there any reason we would want these any less on MathML or whatever
(supposing anyone were to actually use them)?
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Return-value-optimization when return type is RefPtr

[Aryeh Gregor]

On Mon, Aug 15, 2016 at 3:55 PM, Aryeh Gregor  wrote:
> IMO, it makes sense to move ahead with this.

One more point I forgot to (re-?)mention: with proper move
constructors, it's not just already_AddRefed return values that
should be changed to RefPtr, but also T* return values, in the case
where the returning function already had a RefPtr lying around.
This will *reduce* addref/release in common cases, like:

  T*
  Foo()
  {
RefPtr t = DoStuff();
return t;
  }

  RefPtr t = Foo();

Currently this does a release when Foo() returns, and a new addref
when assigning to the new variable.  If Foo() instead returned
RefPtr with proper move constructors, these would be removed.  You
also could not assign the return value of Foo() to a raw pointer
(without .get()), which improves security against use-after-free
without any extra addref/release.  (The release is just handled by the
caller instead of callee.)

It could inadvertently introduce extra addref/release if a function
that doesn't hold a reference on some code paths has its return type
changed to RefPtr, like:

  T* Bar();

  T*
  Foo()
  {
return Bar();
  }

  Foo()->Method();

If Foo()'s return type was changed to RefPtr, this would silently
add an addref/release.  I don't know how to prevent this.  Would
making the relevant constructor explicit do it?  If so, would it cause
issues in other places?

Also -- now I'm remembering more of the issues here -- this would also
require .get() to pass a function's return value directly as a
function parameter, unless bug 1194215 is fixed (see also bug
1194195).  It's not *so* common to do this, but it's common enough
that we don't want to require .get(), I think.

So this isn't such a no-brainer, but I think it is where we ideally want to be.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Return-value-optimization when return type is RefPtr

[Aryeh Gregor]

Sorry for the necroposting, but some points that I think are still
worth correcting:

On Thu, Jun 16, 2016 at 11:50 AM, Boris Zbarsky  wrote:
> On 6/16/16 3:15 AM, jww...@mozilla.com wrote:
>>
>> I think that is the legacy when we don't have move semantics. Returning
>> RefPtr won't incur ref-counting overhead and is more expressive and
>> functional.
>
>
> Except for the footgun described in
> , yes?

This is fixed for (ns)RefPtr in
, and nsCOMPtr
in .  The
footgun code no longer compiles, because Foo() returns RefPtr&& and
will not convert implicitly to T*.

On Thu, Jun 16, 2016 at 5:25 PM, Eric Rescorla  wrote:
> On Thu, Jun 16, 2016 at 2:15 PM, Michael Layzell 
> wrote:
>
>> We pass T* as an argument to a function too often for this to e practical.
>>
>
> Can you explain why? Is your point here to avoid people having to type
> .get() or the mass
> conversion from the current code? The former seems like it's a feature
> rather than a bug...

.get() is basically dangerous and IMO people should not be used to
writing it everywhere.  Passing the pointer from a local RefPtr to a
function without addreffing is not dangerous, because the callee
cannot remove your local strong reference and the caller is guaranteed
to hold the reference until the callee returns.  We should not have to
use .get() everywhere just because the compiler isn't smart enough to
figure out automatically that the operation is actually safe.

I had a proposal (with implementation) for some new parameter types to
replace raw pointers, which would hopefully allow complete removal of
automatic conversion to raw pointer without requiring extensive
.get(), and would also enforce other safety constraints (prohibiting
passing pointers that you don't hold a strong reference to).

  But froydnj
decided he wasn't willing to take them at the present time (and I'm
not sure I disagree).

All of these features are built into Rust, incidentally, because it
tracks lifetimes and will automatically figure out which usages are
safe.

Mass conversion is not an absolute blocker if the change is valuable
enough.  Somebody might know how to automatically rewrite the code,
and if not, I've done pretty large conversions by hand in the past.
(It's just hours of repeatedly recompiling and fixing the same thing
over and over again.)

On Thu, Jun 16, 2016 at 8:40 PM, smaug  wrote:
> Can we somehow guarantee that RVO kicks in? It feels quite error prone to
> rely on compiler optimization when dealing with
> possibly slow Addref/Release calls.

No, RVO is never guaranteed, but move constructors are guaranteed to
be used instead of copy constructors whenever you're returning a local
variable.  Thus we only need to make sure that all cases have move
constructors (including things like moving nsCOMPtr to RefPtr).
We can write tests to make sure that there are no unexpected
AddRef/Release.  I think the required move constructors don't yet
exist, or at least they didn't last I checked, but they could be added
easily.

IMO, it makes sense to move ahead with this.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: invalid values behave like "metadata", not "subtitles"

[Aryeh Gregor]

On Sun, May 8, 2016 at 9:15 AM, Simon Pieters  wrote:
> httparchive (494,168 pages):
>
> SELECT COUNT(*) AS num, REGEXP_EXTRACT(LOWER(body),
> r']+\s)?kind\s*=\s*([a-z]+|["\'][^"\']+["\'])') as match
> FROM [httparchive:har.2016_04_15_chrome_requests_bodies]
> GROUP BY match
> ORDER BY num DESC
>
> Row num match
> 1   17616286null
> 2   523 "subtitles"
> 3   108 "captions"
> 4   58  "metadata"
> 5   6   "subtitle"
> 6   6   'subtitles'
> 7   5   "thumbnails"
> 8   3   'captions'
> 9   1   "dotsub"
> 10  1   "${assettracktype}"
> 11  1   'subtitle'
>
>
> We could add "subtitle" as a new keyword if that turns out to be a problem.

Thanks for the data!  Looks like we're talking on the order of 0.001%
of pages, so I think this can be safely landed.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Intent to ship: invalid values behave like "metadata", not "subtitles"

[Aryeh Gregor]

On Thu, May 5, 2016 at 4:32 PM, Florin Mezei
 wrote:
> Do you still intend to do some analysis to see whether this will be a
> problem in real life? We have somewhat of a history in shipping changes that
> break compatibility, and then end up doing dot releases (in some cases).

I wasn't planning on putting in the work unless others thought it was
really necessary.  In the linked bug, Boris wasn't sure.  I don't know
anything about the feature's deployment, but I'd venture to guess that
it's not so widespread that the number of misuses would be too
significant in absolute terms.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Intent to ship: invalid values behave like "metadata", not "subtitles"

[Aryeh Gregor]

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

Relevant change in standards:
https://github.com/whatwg/html/issues/293, also maybe
https://www.w3.org/2015/10/28-htmlcue-minutes.html

Bugs filed against other UAs:
https://bugs.chromium.org/p/chromium/issues/detail?id=608772
https://bugs.webkit.org/show_bug.cgi?id=157311
https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/7426340/

Invalid values for  are currently treated as "subtitles"
by all UAs, pursuant to old specs.  This means that if we want to add
any new values in the future, old UAs will treat them as subtitles,
which might result in undesired behavior.  As such, several months
ago, the spec was changed to treat invalid values as "metadata", so
UAs will ignore unrecognized values.  No UA has yet implemented the
new spec.

The risk in implementing this is that sites might be inadvertently
using invalid values right now and relying on the fact that they get
treated as "subtitles".  I haven't done any telemetry or other
analysis at this point to gauge if this will be a problem in real
life.

If there are no objections, I intend to land this change on Sunday.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can MAX_REFLOW_DEPTH be increased?

[Aryeh Gregor]

On Mon, May 2, 2016 at 9:58 PM, Boris Zbarsky  wrote:
> For a 900KB stack (what we have on Windows, per
> )
> 512 DOM nodes deep corresponds to ~2KB per DOM node.  200 deep corresponds
> to 5KB.
>
> Now quick, tell me how much stack space MSVC with PGO is using for the
> combination of ElementRestyler::Restyle and ElementRestyler::RestyleChildren
> and ElementRestyler::RestyleContentChildren (which is the unit of recursive
> work when recalculating styles down the tree).  How much stack space will it
> use with the next MSVC release?  How much stack space will it use if we
> change the ElementRestyler struct some (note that this goes on the stack
> before the Restyle() call on it)?
>
> The answer at least for me is that I don't know.  But I bet it's quite
> specific to particular code paths and compilers

Sure.  The same is potentially true if the stack depth limit is 200.
Did anyone do all these calculations to determine that was safe, or
did they just pick a number and see what worked?  By your logic, maybe
the next compiler version will use more stack space and suddenly we'll
start crashing at 200 nodes deep also.

If people are concerned about the possibility that a few weird pages
that are already broken might crash, and nobody wants to do the work
to check this, maybe Chrome (and Safari?) are willing to reduce their
limit to 200.  I don't see why they'd object.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can MAX_REFLOW_DEPTH be increased?

[Aryeh Gregor]

On Mon, May 2, 2016 at 9:19 PM, Steve Fink  wrote:
> It makes me nervous to try to make the overflow behavior the same across
> engines, because it could end up restricting implementation choices. But
> making things roughly the same without trying to make them too much the same
> seems reasonable. And it does sound like there are situations where we just
> outright fail where other browser render *something*, at least, and those
> seem worth improving.

What's the problem with standardizing completely if we pick the limit
to be low enough that we have plenty of room?  If 512 is too high for
us to commit to, we could ask Chrome to drop to 200.  We could also
always change behavior (and maybe the spec) later if it was really
causing a problem.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can MAX_REFLOW_DEPTH be increased?

[Aryeh Gregor]

On Mon, May 2, 2016 at 8:51 PM, L. David Baron  wrote:
> So, at the very
> least, limiting in the parser isn't effective anymore and we need
> checks at later stages, which hopefully could be done in a
> standardizable way.

Having dynamic fallback checks for anything not currently standardized
is of course perfectly sensible.  But we should standardize what we
can.

If the idea is to stop stack overflow, it doesn't make sense to me to
have the check in the parser at all.  It should be on the DOM level.
Otherwise, scripts could make an arbitrarily deep stack, like this:



var cur = document.body;
for (var i = 0; i < 1; i++) {
  var child = document.createElement("span");
  cur.appendChild(child);
  if (child.parentNode != cur) {
break;
  }
  cur = child;
}
document.body.textContent = i;


Which outputs 1.  And indeed, removing the last line of the script
causes an immediate tab crash both in Firefox and Chrome.  So if this
is a security issue, we're vulnerable right now.  If not, we should
probably just standardize the parser limit.  If Firefox and Chrome
both have parser depth limits but not script-exposed depth limits,
probably that's because a non-negligible number of sites historically
hit the parser depth limits, but scripts haven't been much of an
issue.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can MAX_REFLOW_DEPTH be increased?

[Aryeh Gregor]

On Mon, May 2, 2016 at 8:07 PM, Bobby Holley  wrote:
> In general, dynamic stack checks (measuring the top of the stack at XPCOM
> startup, and comparing it with the stack at the point of interest) seem
> preferable to hard-coding number-of-recursive-calls, since it doesn't
> depend on the size of stack frames, which may drift over time. We can't do
> this for JS (see the comments surrounding the MXR link above), but I bet we
> could for layout.

I think we should aim to not make page rendering depend on things like
the size of stack frames or the OS-provided stack size.  :)  We should
pick a value that all UAs can commit to comfortably supporting and
standardize it (as well as standardizing the behavior for how to
process the non-conformant page).  Then if authors write weird pages,
they'll break identically in all browsers.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Can MAX_REFLOW_DEPTH be increased?

[Aryeh Gregor]

Test-case:






var cur = document.querySelector("script");
var depth = -1;
while (cur) {
  cur = cur.parentNode;
  depth++;
}
document.body.textContent = depth;


Outputs 513 in Chrome.  It looks like any further s are inserted
as siblings instead of children.  The  is processed correctly.

IE11 outputs "1003" but then the tab seems to crash.

We don't execute the script at all.  This doesn't seem desirable even
if we do limit the depth.  A modified test-case for us: