Replace Element quickstubs with new binding methods

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted 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
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

Updated

7 years ago
Blocks: 817420
No longer blocks: 817420
Depends on: 817420

Updated

7 years ago
Depends on: 817497
Depends on: 817502
Depends on: 817670
Depends on: 819798
Depends on: 820799
Depends on: 820819

Updated

7 years ago
Depends on: 823526
Depends on: 825605
Depends on: 844781

Updated

5 years ago
Depends on: 1112618
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.