Closed Bug 814195 Opened 8 years ago Closed 8 years ago

Replace Element quickstubs with new binding methods

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(1 file)

Attached patch v1Splinter Review
Still waiting on the final try results, but I think I fixed all the failures.
Attachment #684191 - Flags: review?(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?
Attachment #684191 - Flags: review?(bzbarsky) → review+
(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.
> 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.
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
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.  :(
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...
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
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.  :(
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.
Oh, I lied on the on* props; those are on HTMLElement.  But the rest is still true!
Pushed to try with an attempted fix for that test: https://tbpl.mozilla.org/?tree=Try&rev=73435ee97c1c
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
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
Flags: in-testsuite+
Target Milestone: --- → mozilla20
Blocks: 817420
No longer blocks: 817420
Depends on: 817420
Depends on: 817497
Depends on: 817502
Depends on: 819798
Depends on: 820799
Depends on: 820819
Blocks: 667856
Depends on: 823526
Depends on: 825605
Depends on: 844781
Depends on: 1112618
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.