Closed Bug 761620 Opened 12 years ago Closed 12 years ago

Throw an exception when attempting to add a non-preserved weak map key

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17
Tracking Flags:
Tracking Status
firefox17 + fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Keywords: addon-compat, Whiteboard: [js:t][qa-])

Attachments

(1 file, 1 obsolete file)

throw an exception when an unpreservable wrapped native is used as a weak map key
7.21 KB, patch
billm
: review+
Details | Diff | Splinter Review 
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.
> "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.
(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.
> "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."
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...
Bug 760940 is one example of this.
Whiteboard: [js:t]
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.
Assignee: general → continuation
Attachment #645350 - Flags: feedback?(justin.lebar+bug)
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.
Depends on: 760940
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.
Attachment #645350 - Flags: feedback?(justin.lebar+bug) → feedback+
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
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
(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.
(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.
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
Summary: Make the handling of non-preserved weak map keys more comprehensible → Throw an exception when attempting to add a non-preserved weak map key
> 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
(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.
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.
Depends on: 777373
(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.
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.
Now with proper error messages.
Attachment #645350 - Attachment is obsolete: true
Attachment #649364 - Flags: review?(wmccloskey)
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.
Attachment #649364 - Flags: review?(wmccloskey) → review+
One final full try run, for good luck: https://tbpl.mozilla.org/?tree=Try&rev=f66315a47d2c
https://hg.mozilla.org/mozilla-central/rev/34ae2864bbd8
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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?
tracking-firefox17: --- → ?
Keywords: addon-compat
(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.
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
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.
Weak maps are a new web-exposed JS feature, so potentially anything could use them.
I've backed this out for now: https://hg.mozilla.org/integration/mozilla-inbound/rev/f12bf48505b9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla17 → ---
(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.
(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.
Perfect; thanks a lot.
Tracking for 17 in case this needs to go in after merge.
tracking-firefox17: ? → +
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.
Pushed to try on Linux again to see if anything else has broken in the interim:
https://tbpl.mozilla.org/?tree=Try&rev=3ddd2a3794e1
(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. :)
https://hg.mozilla.org/mozilla-central/rev/236151ae351f
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Whiteboard: [js:t] → [js:t][qa-]
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.
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.
The one that hit us was DOM Event instances.
Okay, thanks.  There's bug 803844 for that specific problem.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: