814195 - Replace Element quickstubs with new binding methods

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Still waiting on the final try results, but I think I fixed all the failures.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Try push at https://tbpl.mozilla.org/?tree=Try&rev=1b1f27da115a looks decent.  ;)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 684191 [details] [diff] [review]
v1

>+++ b/dom/bindings/Bindings.conf
>+    'nativeType': 'mozilla::dom::Element',

Shouldn't be needed; that's the default value.

>+++ b/dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json
>+  "Element interface: attribute attributes": true,

Hmm.  Why was this not needed before?

>+  "Element must be primary interface of element": true,

And this?

>+++ b/js/xpconnect/src/dom_quickstubs.qsconf

     'nsIDOMElement.onmouseenter',
     'nsIDOMElement.onmouseleave',

We should put those in Element.webidl, no?  With a [LenientThis].

r=me with the above dealt with or answered, but we should probably flag this one as addon-compat because of the XUL thing, right?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> >+++ b/dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json
> >+  "Element interface: attribute attributes": true,
> 
> Hmm.  Why was this not needed before?

Because flattening. The failing check is asserting that window.Element.prototype.attributes is an own property, but it lives on window.Node.prototype now.

> >+  "Element must be primary interface of element": true,
> 
> And this?

This checks (IIRC), that window.Element.prototype === element.__proto__, but I guess element.__proto__ still is something XPConnecty.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> asserting that window.Element.prototype.attributes is an own property

Ah, one of the probably-bogus spec changes, ok.

> This checks (IIRC), that window.Element.prototype === element.__proto__

Yeah, ok.

Comment 5

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d70b4460508

Comment 6

[(no longer active)]

Backed out because of test failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/5de5d3950328

Comment 7

[Ed Morley [:emorley]]

Relanded as:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67e95e421678
(In the case of multiple landings and backouts, having this in the bug would make things clearer for sheriffs; if that's ok? :-))

But backed out for intermittent, but frequent (50%+) Windows debug jsreftest failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=.1 mozilla-inbound debug test jsreftest&tochange=28dc63439824&fromchange=6b229b34c046

WinXP variant:
{
 PROCESS-CRASH | file:///C:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_6/extensions/regress-455464-04.js | application crashed (minidump found) 
}

Win7 variant:
{
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_6/extensions/regress-455464-04.js | load failed: null 
}
or
{
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_5/extensions/regress-452168.js | load failed: null
}

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72d38e7a212

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the winxp crash seems to come in several flavors, but they have the following in common:

1)  The actual crash is from CrashingThread.
2)  The main thread is in GC.

So it sounds like we're taking a really long time to GC for some reason?  regress-455464-04.js does something where it tries to gc inside a getter, but it's not even working with Elements, really...  I wonder what's going on there.  :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

So one thing that might be worth looking into is whether commenting out parts of Element still reproduces the jsreftest bustage on try.  Assuming the bustage is reproducible on try at all...

Comment 10

[Andrew McCreight [:mccr8]]

Timeouts in GC can be caused by leaks that grow over time. GCs get longer and longer until eventually it falls over. Printing how long a GCs are taking can be helpful. Maybe something with printStats in js/gc/Statistics.cpp

Comment 11

[Boris Zbarsky [:bzbarsky]]

I pushed some stuff to try:

The patch as landed: https://tbpl.mozilla.org/?tree=Try&rev=1fa6d15956e0
First half of iface: https://tbpl.mozilla.org/?tree=Try&rev=627e876d8004
Second half of iface: https://tbpl.mozilla.org/?tree=Try&rev=a645683010a3

All are green.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, so Peter mentioned that the orange always happens during DefineWebIDLBindingPropertiesOnXPCProto, and I think I know what's going on.  Especially given the data from the retriggers on the try pushes: all properties is almost always orange, but each half is pretty much always green.

With this change, we define a lot more properties on Element.prototype.  In particular, all the on* properties we did not use to define.

Furthermore, we're not doing quickstubs for these properties, so we're using JSNatives, not JSPropertyOps.  That means allocation of actual getter/setter JSFunctions for every property.

The structure of the failing test is basically this:

23   gczeal(2);
25   a=b=c=d=0; this.__defineGetter__('g', gc); for each (y in this);
27   gczeal(0);

So it's enumerating the window object, which resolves all interface objects on the window nowadays.  And resolving the interface objects sets up its .prototype.  And that will create a slew of JSFunctions with this patch, when Element.prototype is set up.  And because of the gczeal thing every single object allocation will gc.  Which will take a long time, and sometimes trigger our hang detector.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Oh, I lied on the on* props; those are on HTMLElement.  But the rest is still true!

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed to try with an attempted fix for that test: https://tbpl.mozilla.org/?tree=Try&rev=73435ee97c1c

Comment 15

[Boris Zbarsky [:bzbarsky]]

Looks like we have another test that does something similar.  Pushed to try with an attempted fix for both; grep for "in this" and "gczeal" suggests there should be no more: https://tbpl.mozilla.org/?tree=Try&rev=a6d0f739651b

Comment 16

[Boris Zbarsky [:bzbarsky]]

That looks green, so landed the test fixes and relanded the patch:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a76899feb06
   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1425a30de0e

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a76899feb06
https://hg.mozilla.org/mozilla-central/rev/c1425a30de0e