Last Comment Bug 761620 - Throw an exception when attempting to add a non-preserved weak map key
: Throw an exception when attempting to add a non-preserved weak map key
Status: RESOLVED FIXED
[js:t][qa-]
: addon-compat
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on: 760940 777373 781619
Blocks: WeakMap
  Show dependency treegraph
 
Reported: 2012-06-05 08:11 PDT by Andrew McCreight [:mccr8]
Modified: 2012-10-24 11:11 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
throw an exception when an unpreservable wrapped native is used as a weak map key (3.51 KB, patch)
2012-07-24 09:41 PDT, Andrew McCreight [:mccr8]
justin.lebar+bug: feedback+
Details | Diff | Splinter Review
throw an exception when an unpreservable wrapped native is used as a weak map key (7.21 KB, patch)
2012-08-06 12:51 PDT, Andrew McCreight [:mccr8]
wmccloskey: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-06-05 08:11:40 PDT
For technical reasons, the only wrapped natives that can be used as weak map keys are those that can have their wrappers preserved.  For simplicity, I limited this to nodes. (Initially I allowed all wrapper cached objects, but it turns out there are two types of wrapper cached objects that don't actually preserve their wrappers.)

The current handling of other wrapped natives is flawed in one or two ways.

First, the error message, "Failed to preserve wrapper of wrapped native weak map key", is completely incomprehensible.  jlebar and mak have hit it and had no idea what it meant, and if they can't figure it out, there's no way any random person writing JS is going to understand what it meant.  I'd welcome suggestions for the error message.  I'm not sure how to talk about "non-node wrapped natives" in a way that might make sense to a JS programmer.

Secondly, I believe that the current behavior tries to be nice by going ahead and adding the key anyways but this is going to make the behavior awful, because they will be able to add something, immediately test to see if it is still there and see that it is, but then if they go and try to use it much later it will probably not be there any more.  So, this should probably not add a key if wrapper preservation fails.  I should add a test for that, too.
Comment 1 Justin Lebar (not reading bugmail) 2012-06-05 08:19:37 PDT
> "Failed to preserve wrapper of wrapped native weak map key"

How about

  "Cannot use given object as a weak map key."

It might be better to add some explanation to the error message; what kinds of things can a web developer not add?

I also wonder if this should be an exception, rather than a warning.  That's probably a question for the spec, though.

In any case, I agree that if you don't ever add the item to the weak map, that will make the behavior more noticeable.
Comment 2 Marco Bonardo [::mak] 2012-06-05 08:28:11 PDT
(In reply to Andrew McCreight [:mccr8] from comment #0)
> First, the error message, "Failed to preserve wrapper of wrapped native weak
> map key", is completely incomprehensible.

Well, the fact itself it talks about "preserving a wrapper" makes it comprehensible only to someone who wrote the patch.  No idea what preserving a wrapper means.
And the fact it's a simple warning makes me think it doesn't matter for functionality, like "ok you can't preserve this wrapper, maybe you will be slower? or use more memory?".

I agree this should not be a warning, it may actually hide lots of bugs, but if it has to be "Using wrapped natives as weak map keys is unsupported and discouraged" would make it more comprehensible.
Comment 3 Justin Lebar (not reading bugmail) 2012-06-05 08:37:18 PDT
> "Using wrapped natives as weak map keys is unsupported and discouraged"

But "unsupported and discouraged" could mean exactly the same thing you were talking about -- it might work, maybe it's slower or uses more memory.

We should say it *does not work*, because that's what we mean.  "Wrapped natives cannot be used as weak map keys."  Except that's not actually what the condition is; you cannot use a *certain type* of wrapped natives.

So like I said, instead of trying to explain the innards of xpconnect inside an error message, I think we should just say "you can't use this object as a key to the weak map."
Comment 4 Andrew McCreight [:mccr8] 2012-06-05 08:47:49 PDT
Technically, it will work as long as you keep a reference to the C++ object from JS.  But that isn't useful in any scenario where you want to use a weak map...
Comment 5 Andrew McCreight [:mccr8] 2012-06-05 11:50:50 PDT
Bug 760940 is one example of this.
Comment 6 Andrew McCreight [:mccr8] 2012-07-24 09:41:48 PDT
Created attachment 645350 [details] [diff] [review]
throw an exception when an unpreservable wrapped native is used as a weak map key

With this patch, if the user attempts to add a weak map key that is a wrapped native, but not preservable, it throws an exception "Cannot use the given object as a weak map key.", and does not add it to the map.  How does that sound to you, Justin, from a user perspective?

I added some tests to ensure we throw in these situations.
Comment 7 Andrew McCreight [:mccr8] 2012-07-24 09:48:03 PDT
This will probably break the tree until bug 760940 is fixed (it is on inbound).

Try run to (hopefully) confirm that these tests will break with my patch:
https://tbpl.mozilla.org/?tree=Try&rev=52e4c776afe1

...and to see if there's any other lurking badness.
Comment 8 Justin Lebar (not reading bugmail) 2012-07-24 09:49:31 PDT
Comment on attachment 645350 [details] [diff] [review]
throw an exception when an unpreservable wrapped native is used as a weak map key

> How does that sound to you, Justin, from a user perspective?

Really good.
Comment 9 Dave Herman [:dherman] 2012-07-24 10:12:16 PDT
Trying to understand, does this affect content? More to the point, I'm trying to figure out what, if anything, ES6 should have to say about invalid keys.

Thanks,
Dave
Comment 10 Dave Herman [:dherman] 2012-07-24 10:13:51 PDT
But I agree that from a user's perspective, rejecting keys that definitely can't be held weakly is the right approach. In particular, value objects (see bug 749786) should be rejected as keys in WeakMaps.

Dave
Comment 11 Andrew McCreight [:mccr8] 2012-07-24 10:18:03 PDT
(In reply to Dave Herman [:dherman] from comment #9)
> Trying to understand, does this affect content?
Yes, it does.

> More to the point, I'm trying to figure out what, if anything, ES6 should have to
> say about invalid keys.
That's a good question.  These objects (non-preservable wrapped natives) are going to have weird behavior in other places, too.  I believe that if you try to set random properties on them ("expandos"), those extra properties can go away without warning during a GC.  I don't know if the spec has anything to say about that.

Right now, we only allow wrapped natives that are nodes, for simplicity.  In the future, as more things are hooked up to the new DOM bindings, I believe that more things will be preservable, so maybe this restriction could be loosened a bit.
Comment 12 Andrew McCreight [:mccr8] 2012-07-24 10:21:17 PDT
(In reply to Dave Herman [:dherman] from comment #10)
> But I agree that from a user's perspective, rejecting keys that definitely
> can't be held weakly is the right approach. In particular, value objects
> (see bug 749786) should be rejected as keys in WeakMaps.

The particular problem here is that these keys are held too weakly: if they are placed in a weak map, they can get randomly removed from the weak map during a GC, even if they are still alive.  The current behavior is particularly bad because if you look right away, it will still be there, but later it may not be.

It is also a bit ugly because "non-preservable wrapped natives" are not really a JS language concept, they are an implementation detail, so I'm not really sure how to give a better error message.
Comment 13 Andrew McCreight [:mccr8] 2012-07-24 12:47:34 PDT
As expected, a ton of places tests fail with this patch, but without the patch in bug 760940.  There are also a few failures in highlighter and style inspector tests.  I'm going to push again with the places patch and see what happens.  The non-Moth tests seem okay.

https://tbpl.mozilla.org/php/getParsedLog.php?id=13812103&tree=Try#error80
Comment 14 Dave Herman [:dherman] 2012-07-24 13:49:43 PDT
> It is also a bit ugly because "non-preservable wrapped natives" are not
> really a JS language concept, they are an implementation detail, so I'm not
> really sure how to give a better error message.

So the next question is how can you characterize what these "non-preservable wrapped natives" (NPWN's) are, so that we can actually help users to understand what they are, how they come about, what they can't do with them, and how to work around it. Can you give a characterization?

As for specification, don't worry too much about that. If you can help me understand what's going on I can figure out how much to specify and how.

Thanks,
Dave
Comment 15 Marco Bonardo [::mak] 2012-07-25 05:32:26 PDT
(In reply to Andrew McCreight [:mccr8] from comment #13)
> As expected, a ton of places tests fail with this patch, but without the
> patch in bug 760940.

That bug landed yesterday on m-i, should be merged soon to m-c and is already approved for Aurora.
Comment 16 Andrew McCreight [:mccr8] 2012-07-25 06:02:36 PDT
Yeah, I was mostly just using that as a test to make sure my patch was working.

There's also a place in the style inspector that is incorrectly using weak map keys.  I'll file a bug on that today at some point.

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_ruleviewstore.js | an unexpected uncaught JS exception reported through window.onerror - Error: Cannot use the given object as a weak map key. at resource:///modules/devtools/CssRuleView.jsm:490
Stack trace:
    JS frame :: chrome://mochikit/content/tests/SimpleTest/SimpleTest.js :: simpletestOnerror :: line 994
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

JavaScript error: resource:///modules/devtools/CssRuleView.jsm, line 490: Error: Cannot use the given object as a weak map key.
Comment 17 Andrew McCreight [:mccr8] 2012-07-25 09:21:17 PDT
(In reply to Dave Herman [:dherman] from comment #14)
> So the next question is how can you characterize what these "non-preservable
> wrapped natives" (NPWN's) are, so that we can actually help users to
> understand what they are, how they come about, what they can't do with them,
> and how to work around it. Can you give a characterization?

I'm not really sure.  First of all, I don't know how you'd characterize the difference between something implemented in C++ and exposed to JS and something that is just pure JS.  I think if you can do that, you can just say, "well, of the former category, only nodes are supported" to describe the current behavior.  If we broaden what is allowed, then it will be harder to characterize.
Comment 18 Andrew McCreight [:mccr8] 2012-08-03 10:48:34 PDT
https://tbpl.mozilla.org/?tree=Try&rev=9235230ab264
Comment 19 Andrew McCreight [:mccr8] 2012-08-04 10:35:19 PDT
That try run and a separate one on Android came out okay, so I just need to fix up the error messages to be proper-like then I can put it up for review.
Comment 20 Andrew McCreight [:mccr8] 2012-08-06 12:51:56 PDT
Created attachment 649364 [details] [diff] [review]
throw an exception when an unpreservable wrapped native is used as a weak map key

Now with proper error messages.
Comment 21 Bill McCloskey (:billm) 2012-08-06 14:48:31 PDT
Comment on attachment 649364 [details] [diff] [review]
throw an exception when an unpreservable wrapped native is used as a weak map key

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

::: js/src/jsweakmap.cpp
@@ +250,5 @@
>      }
>  
> +    // Preserve wrapped native keys to prevent wrapper optimization.
> +    if (key->getClass()->ext.isWrappedNative) {
> +        if (!cx->runtime->preserveWrapperCallback) {

As we discussed, let's assume this is present and assert if it isn't. Then the NO_PRES_WRAPPER error won't be needed.
Comment 22 Andrew McCreight [:mccr8] 2012-08-07 10:55:14 PDT
One final full try run, for good luck: https://tbpl.mozilla.org/?tree=Try&rev=f66315a47d2c
Comment 23 Andrew McCreight [:mccr8] 2012-08-08 11:17:10 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/34ae2864bbd8
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-08-08 18:24:38 PDT
https://hg.mozilla.org/mozilla-central/rev/34ae2864bbd8
Comment 25 Dave Townsend [:mossop] 2012-08-14 10:45:58 PDT
This is breaking SDK based add-ons right now and could also be affecting other add-ons. Even if we fixed the SDK today there would still be a lag (significant right now) in getting existing add-ons updated. I'd like to ask that we back this out for the time being until we at least have a fix on the SDK side.

Perhaps it would also make sense to turn this into a console warning instead of throwing an exception for a release or two of Firefox so add-on/web developers will at least have some warning that this is coming?
Comment 26 Andrew McCreight [:mccr8] 2012-08-14 11:34:04 PDT
(In reply to Dave Townsend (:Mossop) from comment #25)
> This is breaking SDK based add-ons right now and could also be affecting
> other add-ons. Even if we fixed the SDK today there would still be a lag
> (significant right now) in getting existing add-ons updated. I'd like to ask
> that we back this out for the time being until we at least have a fix on the
> SDK side.
> 
> Perhaps it would also make sense to turn this into a console warning instead
> of throwing an exception for a release or two of Firefox so add-on/web
> developers will at least have some warning that this is coming?

This has produced a console warning (albeit a weird and confusing one...) since bug 680937 landed almost exactly a year ago, so I don't know if just reverting the patch for another cycle or two is going to help. Anybody who hasn't fixed things by now probably isn't going to fix things until they see the exceptions.

Anyways, I'm happy to back this out to give you longer to fix this.

Another approach would be to just remove the part where we throw an exception, and instead warn. So if you try to add a bad key, I'd change it to produce a warning, then never add it, instead of adding it in a way that would lead to it being silently removed at some future point. That would be a change in behavior, but maybe would make it easier to find and fix these errors.
Comment 27 Andrew McCreight [:mccr8] 2012-08-14 11:48:30 PDT
To summarize:

old behavior, since Fx 11: warn, but add anyways, may disappear randomly later
Fx 17 (with the patch here): throw an exception, don't add it
possible middle ground: warn, don't add it
Comment 28 Jorge Villalobos [:jorgev] 2012-08-14 16:38:31 PDT
The console warning might not do much, but updating the SDK and maybe warning SDK developers would go a long way in reducing its impact. It doesn't sound like something that would affect many XUL add-ons, but I'm not entirely sure what a developer would need to do to trigger this.
Comment 29 Andrew McCreight [:mccr8] 2012-08-14 16:48:06 PDT
Weak maps are a new web-exposed JS feature, so potentially anything could use them.
Comment 30 Dave Townsend [:mossop] 2012-08-15 10:32:07 PDT
I've backed this out for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/f12bf48505b9
Comment 31 Justin Lebar (not reading bugmail) 2012-08-15 10:36:32 PDT
(In reply to Dave Townsend (:Mossop) from comment #30)
> I've backed this out for now:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f12bf48505b9

What's your plan for getting this back in, Dave?  This is a badly-needed change, which exposes bugs in add-ons, jetpack, and webpages which have been ignored for a full year.

If we're holding this for a week or two while you ensure that the current version of Jetpack is fixed, that sounds good to me.  If on the other hand you intend to hold this for multiple cycles while we figure out how to fix old versions of jetpack, that does not sound good to me.
Comment 32 Dave Townsend [:mossop] 2012-08-15 10:41:28 PDT
(In reply to Justin Lebar [:jlebar] from comment #31)
> (In reply to Dave Townsend (:Mossop) from comment #30)
> > I've backed this out for now:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/f12bf48505b9
> 
> What's your plan for getting this back in, Dave?  This is a badly-needed
> change, which exposes bugs in add-ons, jetpack, and webpages which have been
> ignored for a full year.

The plan is to get bug 781619 fixed as soon as possible, Alex who wrote the code that seems to be most affected by this, is back from vacation on Monday and it will be his top priority. Once it's done I'll probably see if we can do a hotfix release of the SDK with the fix in it, then I think we're in a better place to land this back on m-c. Hopefully it will be before the next aurora merge.
Comment 33 Justin Lebar (not reading bugmail) 2012-08-15 11:22:10 PDT
Perfect; thanks a lot.
Comment 34 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-15 13:52:22 PDT
Tracking for 17 in case this needs to go in after merge.
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-08-15 18:40:31 PDT
(In reply to Dave Townsend (:Mossop) from comment #30)
> I've backed this out for now:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f12bf48505b9

https://hg.mozilla.org/mozilla-central/rev/f12bf48505b9
Comment 36 Wes Kocher (:KWierso) 2012-08-20 18:27:20 PDT
So, we backed out the SDK change that started using weakmaps in bug 764831, and I just pushed a change in bug 784113 to mozilla-inbound that should pick up that change in the testsuite.

Looks like everything in that push is passing except for the unrelated OSX opt breakage, I think we're good to reland this.
Comment 37 Andrew McCreight [:mccr8] 2012-08-21 13:21:45 PDT
Pushed to try on Linux again to see if anything else has broken in the interim:
https://tbpl.mozilla.org/?tree=Try&rev=3ddd2a3794e1
Comment 38 Andrew McCreight [:mccr8] 2012-08-23 09:06:17 PDT
Relanded
https://hg.mozilla.org/integration/mozilla-inbound/rev/236151ae351f
Comment 39 Wes Kocher (:KWierso) 2012-08-23 09:58:06 PDT
(In reply to Andrew McCreight [:mccr8] from comment #38)
> Relanded
> https://hg.mozilla.org/integration/mozilla-inbound/rev/236151ae351f

Jetpack tests seem to be passing this time. :)
Comment 40 Ryan VanderMeulen [:RyanVM] 2012-08-23 19:20:19 PDT
https://hg.mozilla.org/mozilla-central/rev/236151ae351f
Comment 41 Erik Arvidsson 2012-10-22 12:17:33 PDT
Is there a bug for this invalid behavior? Not being able to rely on WeakMaps when I know the key is an object is pretty bad.
Comment 42 Andrew McCreight [:mccr8] 2012-10-22 12:44:35 PDT
What kind of objects are you trying to use as keys?  Pure JS objects should be okay right now, but objects that are implemented under the hood are not well supported for technical reasons. However, we should be able to broaden the class of things that work a little more right now, which is I guess is in bug 777385.
Comment 43 Erik Arvidsson 2012-10-24 10:51:21 PDT
The one that hit us was DOM Event instances.
Comment 44 Andrew McCreight [:mccr8] 2012-10-24 11:11:53 PDT
Okay, thanks.  There's bug 803844 for that specific problem.

Note You need to log in before you can comment on or make changes to this bug.