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

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: khuey, Assigned: bzbarsky)

Tracking

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [MemShrink:P1])

Attachments

(3 attachments, 1 obsolete attachment)

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.
Whiteboard: [MemShrink:P1]
Nice bug.
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.
I've verified that both tests fail without the patch
Attachment #8394234 - Flags: review?(khuey)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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...
Attachment #8394234 - Attachment is obsolete: true
Attachment #8394234 - Flags: review?(khuey)
This fixes everything except navigator.mozPay, which is directly exposing chrome objects to content... We should get it fixed.
Attachment #8394250 - Flags: review?(khuey)
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.
Attachment #8394250 - Flags: review?(khuey) → review+
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?
Flags: in-testsuite+
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.
Please request approval-mozilla-b2g28 on this patch when it is ready for uplift.
Flags: needinfo?(bzbarsky)
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
Working on it, yes.  Figured I should make sure it compiles before posting it.
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.
Attachment #8395076 - Flags: approval-mozilla-b2g28?
Flags: needinfo?(bzbarsky)
Attachment #8395076 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+
Depends on: 1245313
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.