935283 - Bug 932322 regressed the testcase for Bug 916851 significantly (window.document access regression)

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

https://bugzilla.mozilla.org/show_bug.cgi?id=932322#c15

Comment 1

[Matt Brubeck (:mbrubeck)]

This also shows up as a regression in the Dromaeo benchmark.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So in the short term we may just need to put the manual expando thing back.

In the medium term, when we switch window to webidl the overhead here will drop drastically.  We won't be able to mark it [Constant], but we'll be able to mark it [Pure]... not that this would help in the cases which do method calls on the document.  :(

In the long term, I'm not sure what the right plan is.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Perhaps a silly idea...
since .document is rather important to be as fast as possible, and its value doesn't change
too often, could we special case its getter.

in pseudojs something like
// unset unforgeability
(function (d) {
  window.__defineGetter__("document", function() { return d;});
})(newDoc);
//set back the unforgeability flag
Wouldn't that let jseng to optimize the getter better than in case we call to native code?

On C++ side there would be hopefully something like
WindowBinding::UpdateDocument(nsGlobalWindow* aThis, nsIDocument* aNewDoc);

Comment 5

[Boris Zbarsky [:bzbarsky]]

>// unset unforgeability

You can't actually do that.  The whole point of unforgeability is you can't mess with the getter, and if you could unset it that would defeat the whole point...

What we _can_ maybe do if we want is to adopt an approach similar to what I've advocated for ImageDaga.data: store the relevant JSObject* or JS::Value (in this case the wrapper for the document) in the C++ object we're getting from (in this case nsGlobalWindow) and have the generated code generate an offsetof value that it stashes in the jitinfo.  Then have the JIT just read the value directly from the C++ object.  I think we'd still need to pay the ExposeObjectToActiveJS cost, and do it manually in jitcode, but that would be about it.

We should really prototype that with imagedata...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> >// unset unforgeability
> 
> You can't actually do that.  The whole point of unforgeability is you can't
> mess with the getter, and if you could unset it that would defeat the whole
> point...
I was thinking some js engine magic, not anything exposed to js.

Comment 7

[Boris Zbarsky [:bzbarsky]]

It would be exposed, since the object identity of the getter function would change with your proposal above, and you can get that function object via getOwnPropertyDescriptor().get.

Comment 8

[Peter Van der Beken [:peterv]]

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

Let's do this for now.

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

    https://hg.mozilla.org/integration/mozilla-inbound/rev/faee77ddc112

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-4 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea83604de5e3

https://tbpl.mozilla.org/php/getParsedLog.php?id=30232433&tree=Mozilla-Inbound

Comment 12

[Ryan VanderMeulen [:RyanVM]]

And https://tbpl.mozilla.org/php/getParsedLog.php?id=30232701&tree=Mozilla-Inbound

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v2

We need to make document non-unforgeable, so I decided to remove the WebIDL property for now. Looks pretty green on try.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 828751 [details] [diff] [review]
v2

r=me

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

would this bug be the reason for dromaeo DOM (query.html, modify.html) regressing around November 5th:
https://datazilla.mozilla.org/?start=1383306067&stop=1383910867&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.2.9200&test=dromaeo_dom&graph_search=c9eb4218558d&tr_id=3460240&graph=modify.html&x86=false&error_bars=false&project=talos

according to the graphs this is the offender: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=c9eb4218558d, yet there were 5 pushes prior that didn't build or run windows 8.

here is a link to the entire set of changes via tbpl on Nov 5th:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&startdate=2013-11-05&enddate=2013-11-06

Comment 16

[Boris Zbarsky [:bzbarsky]]

Yes, it would.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b539bdc3e6b7

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b539bdc3e6b7