1197096 - WeakMap constructor throws TypeError from wrong realm

Status: REOPENED

(Core :: JavaScript: Standard Library, defect, P5)

Description

[André Bargull [:anba]]

Test case:
---
var g = newGlobal();
WeakMap.prototype.set = g.WeakMap.prototype.set;

try {
  new WeakMap([[0, 0]]);
} catch (e) {
  assertEq(e instanceof g.TypeError, true);
}
---


Expected: The assertion passes
Actual: Throws `Assertion failed: got false, expected true`

Comment 1

[David Bruant]

What is 'newGlobal'?
Is the test case really related to the standard part of the language?
Is this problem observable from the web?

Comment 2

[Till Schneidereit [:till]]

(In reply to David Bruant from comment #1)
> What is 'newGlobal'?

A built-in function of the js shell that creates a new global object, with its own set of builtins.

> Is the test case really related to the standard part of the language?

Yes.

> Is this problem observable from the web?

Yes, the same would happen if you replaced newGlobal() with a reference to some iframe in a document with iframes.

Comment 3

[Till Schneidereit [:till]]

(In reply to Till Schneidereit [:till] from comment #2)
> > Is the test case really related to the standard part of the language?
> 
> Yes.

And now I'm not so sure about this part. André, can you point me to where the spec defines which realm the TypeError should be created in? It *seems* like %throwtypeerror%[1] was an attempt at doing something realm-specific here, but that's very rarely used, and not really clear in how it's supposed to work.

My best guess would be that the error instance is always created in the current execution context[2]. However, I don't see anything in the spec that would say how the current execution context relates to builtins. Our behavior for builtins is to essentially always activate the execution context the builtin was defined in, so in this case WeakMap.prototype.set does the expected thing in throwing an instance from the global it originated from. This seems to match the behavior specified in [[Call]][3], which doesn't differentiated between builtins and content functions. I guess another concept that would make sense is to only take the last non-builtin frame on the stack into consideration. Perhaps the spec mandates that somewhere and I just can't find it?


[1] http://www.ecma-international.org/ecma-262/6.0/index.html#sec-%throwtypeerror%
[2] http://www.ecma-international.org/ecma-262/6.0/index.html#sec-execution-contexts
[3] http://www.ecma-international.org/ecma-262/6.0/index.html#sec-built-in-function-objects-call-thisargument-argumentslist

Comment 4

[Jeff Walden [:Waldo]]

It's not well-defined, I expect, but I think I agree that this is a bug on our part.

We have, in G1, |new WeakMap|.  That delegates to G2's WeakMap.prototype.set, for setting.  If we perform the set in G2, we get the right behavior.  But WeakMap_set uses CallNonGenericMethod, to unwrap |this| which is a WeakMap from G1.  So by the time we're doing the actually set-implementation work, we're back in G1 again, and the thrown exception is from G1.

Admittedly the exact global of "throw a TypeError" language is not precise, but in practice I think it means the global of the caller.  Under that meaning, adhered to pretty widely everywhere else, I think we're buggy.

Comment 5

[Jeff Walden [:Waldo]]

Er, the global of the function currently being called, that is G2's WeakMap.prototype.set.

Comment 6

[Till Schneidereit [:till]]

We talked about this on IRC, and I agree with Waldo's conclusion.

One solution would be to add an errorReportingGlobal to the NativeImpl signature. CrossCompartmentWrapper::nativeCall could pass in the original global, while the CallNonGenericMethod could pass null, indicating that the current global is the right one.

This still seems somewhat fragile to me, but I can't think of anything more robust: we can't add asserts about any pending exceptions coming from the right compartment because nested invocations of content scripts might create them pretty much anywhere.

Comment 7

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #3)
> It *seems* like %throwtypeerror%[1] was an attempt at doing something realm-specific
> here, but that's very rarely used, and not really clear in how it's supposed
> to work.

%ThrowTypeError% is only used for the poison-pilled ".arguments" and ".caller" properties. 

 
> My best guess would be that the error instance is always created in the
> current execution context[2]. 

Yes, this is the general agreement. I didn't find an explicit note in the spec which states this behaviour, though. 


> However, I don't see anything in the spec that
> would say how the current execution context relates to builtins. [...] This seems to
> match the behavior specified  in [[Call]][3], which doesn't differentiated between
> builtins and content functions. 

Built-in functions use either "9.2.1 [[Call]]" or "9.3.1 [[Call]]", but in both cases a new ECMAScript code execution context is pushed on the execution context stack. So when WeakMap.prototype.set is called in the test case, the current Realm is different from the Realm of the WeakMap constructor. 


Having said that, I have to admit I didn't realize this issue was caused by CallNonGenericMethod. I simply assumed this fast path [1] was responsible for the realm mismatch and then I stopped investigating. Fixing CallNonGenericMethod is a more difficult (and time consuming) task. Ugh. :-/

[1] http://hg.mozilla.org/mozilla-central/file/23a04f9a321c/js/src/jsweakmap.cpp#l540

Comment 8

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.