985827 - Extending Navigator with NewResolve is totally broken in the presence of xrays

Status: RESOLVED FIXED

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

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

DOM Xrays invoke DoNewResolve unconditionally, which means we create a new copy of the object every time.  We actually do it twice, too, since we invoke DoNewResolve on the object itself and then again on the xray wrapper.  We also end up with properties defined on the Navigator object itself instead of the proto.  I don't know where to start untangling this mess, but this is causing the memory leaks that we see in bug 979173 and bug 981871.

Comment 1

[Andreas Gal :gal]

Nice bug.

Comment 2

[Boris Zbarsky [:bzbarsky]]

So... we did know this.  It's broken with non-Xrays too, given 'delete' behavior.  Basically, if you use a resolve hook you are NOT guaranteed the operation will only happen once.  The only thing that happens with Xrays is that this guarantee is always violated, whereas for non-Xrays it's violated whenever the page wants it to be violated.

> We also end up with properties defined on the Navigator object itself

Yes, that's how resolve hooks work.  This is not Xray-specific.

> We actually do it twice, too

Hmm.  I thought we'd special-cased Navigator here, or at least I vaguely recall there being some problem we considered doing that for, but I'm not seeing anything in the code or in bug 945416.  Maybe the discussion was on IRC.  :(

In any case, I think the simple and safe fix here is to just add a hashtable to Navigator that maps property names to objects.  Then we store the value in the hashtable.  On newresolve, we do a hashtable lookup before creating a new object.  That will ensure that the object is only created once.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Navigator::DoNewResolve not double-create objects no matter what JS is doing.

I've verified that both tests fail without the patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8394234 [details] [diff] [review]
Make Navigator::DoNewResolve not double-create objects no matter what JS is doing.

Er, no, the interactions with the gpi crud are wrong here...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Make Navigator::DoNewResolve not double-create objects no matter what JS is doing.

This fixes everything except navigator.mozPay, which is directly exposing chrome objects to content... We should get it fixed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Same thing as diff -w for review convenience

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8394250 [details] [diff] [review]
Make Navigator::DoNewResolve not double-create objects no matter what JS is doing.

Review of attachment 8394250 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/test/test_navigator_resolve_identity.html
@@ +24,5 @@
> +  is(typeof x, "object", "Should have a mozApps object");
> +  delete navigator.mozApps;
> +  y = navigator.mozApps;
> +  is(x, y, "Should have gotten the same mozApps object again");
> +  

nit: whitespace at EOL (and extra \n)

::: dom/base/test/test_navigator_resolve_identity_xrays.xul
@@ +20,5 @@
> +  <!-- test code goes here -->
> +  <script type="application/javascript">
> +  <![CDATA[
> +  /** Test for Bug 985827 **/
> +  

and whitespace here too.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5dd0ed17d95 with those fixes, and then https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe84f49b0ea to nix the stray shadowing Rooted.  :(

Which Gecko branches do we want/need this on?

Comment 9

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/c902425901af to fix the issue with caching objects wrapped into the Xray compartment.  We had something to avoid that, but only in the webidl case, not the navigator category case.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5dd0ed17d95
https://hg.mozilla.org/mozilla-central/rev/9fe84f49b0ea
https://hg.mozilla.org/mozilla-central/rev/c902425901af

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please request approval-mozilla-b2g28 on this patch when it is ready for uplift.

Comment 12

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

We need a patch which applies to v1.3 tip cleanly [1] . Can we have a patch for that quickly ?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=981871#c77

Comment 13

[Boris Zbarsky [:bzbarsky]]

Working on it, yes.  Figured I should make sure it compiles before posting it.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: b2g28 patch

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8395076 [details] [diff] [review]
b2g28 patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probably been broken for a good long while, in the Xray case.
User impact if declined: Use too much memory. 
Testing completed: Compiles.
Risk to taking this patch (and alternatives if risky): Medium risk, though I think I did the merging correctly.
String or UUID changes made by this patch: None.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/381d797b8dff
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/d62897f5a25f