1516691 - Consider making the following a release assertion: xpc::IsInContentXBLScope(obj) || !xpc::UseContentXBLScope(JS::GetObjectRealmOrNull(obj)), at src/dom/base/nsINode.cpp:2664

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Alex Gaynor [:Alex_Gaynor]]

Bug 1516289 is the Nth iteration we have of hitting this debug assertion. When it's hit, it generally indicates a sec-high issue as I understand it.

We should consider making this a release assert, which will render this condition unexploitable, and also assist in discovering bugs where we violate it.

I did a try run of this, results here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=825144742ca907dc082b5d8f547fde631dc9c7c5&framework=1&showOnlyComparable=1&selectedTimeRange=172800

There are two negative results in the >2% range, both of which are Windows10-64bit only. There are a handful of other medium confidence results which are not large.

Emilio was concerned that we might not have any talos benchmarks that would catch this; :bz, do you know if that's the case?

Assuming we do have benchmarks that'd catch a regression caused by this change, do the results seem reasonable to land?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Apart from maybe dromaeo I don't think we have fine-grained DOM performance benchmarks.  Do we even still run dromaeo on talos?  People kept wanting to turn it off...

If we do run it, you'd need to look at the specific subtests that would hit this code (i.e. ones that would involve returning nodes to JS).

Another interesting question is this: given the on-the-chopping-block status of content XBL scopes, will this assert even stick around in current form for much longer?

Comment 2

[Alex Gaynor [:Alex_Gaynor]]

Looking at the dromaeo DOM subtests: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=825144742ca907dc082b5d8f547fde631dc9c7c5&originalSignature=91ee66e069fdeb63f2aaabf8b4cff8451273c461&newSignature=91ee66e069fdeb63f2aaabf8b4cff8451273c461&framework=1&selectedTimeRange=172800

They only seem to run on Linux, there are small regresionss, from 0-4%, all at low confidence (with the noise metric way up).

Emilio, do you know about whether this assert will still exist with the demise of content XBL?

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

It looks like https://hg.mozilla.org/mozilla-central/rev/9f776274089a changed this assert slightly.

Is it still security sensitive?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Yes, I think so.

Comment 5

[Alex Gaynor [:Alex_Gaynor]]

Ok, I'll rebase then.

Comment 6

[Alex Gaynor [:Alex_Gaynor]]

Attachment: Bug 1516691 - use a release assert to prevent this scenario in release builds; r?emilio

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/40545233f9fec17e4dfdd766353f1910bb5c9030

Comment 8

[Julien Cristau [:jcristau] (back April 22)]

https://hg.mozilla.org/mozilla-central/rev/40545233f9fe