Now that bug 870514 is fixed we have an IC which does the basic "call into the proxy handler" thing. Unfortunately, it's only used in some cases: specifically the "listbase and we're shadowing the property" cases. djvj thinks we should generally reorganize the proxy IC code, and I think I agree: it's sort of interwingled with the other IC code right now. In what follows, I'll refer to the DOM proxies as "listbase" because that's what we do, though I think we should rename that to "DOMProxy". I believe we have the following cases for proxies in general: 1) Not a listbase. We can't optimize these too much past just getting into their proxy handler as quickly as possible. Essentially the codepath that bug 870514 added, but with just a guard that says "proxy and not listbase" or something. 2) Listbase which returns "DoesntShadow" from the ListBaseShadowsCheck call. We already have an IC for this, but the basic idea is that we should guard on the expando object's existence and shape and then just look for the property on the proto. If it's found there, do whatever guards are needed to ensure that keeps happening and then use the result. If it's _not_ found on the proto, that doesn't mean the proxy itself won't have it, though. As a first cut, we want to fall back to the same codepath as item 1. Longer-term it might be nice to indicate to the proxy somehow that we're only looking for own properties at this point... Right now if the proto does not have the property we keep trying to add stubs (see bug 870508). 3) Listbase which returns "Shadows". This is just like item 1: we just want to get into the proxy handler as fast as we can. We presumably want to guard on having a listbase and the expando object bits and for OverrideBuiltins proxies the generation counter, I guess. 4) Listbase which returns "DoesntShadowUnique". This is just like item 2 in terms of the code we want to do, but we need to guard on being a listbase, the expando object bits, and the generation counter, similar to item 3's OverrideBuiltins case. That's all for named access, of course. Indexed access is an entirely separate kettle of fish, though again for non-listbase proxies we just want to land in the handler as fast as we can for that too. As for listbase proxies and indexed access, the shadowing rules are actually different for indexed vs named access; right now our shadowing check assumes named access. Ideally we'd just do different checks here when we know the propid is an index....

One other note. There _is_ another special case which it would be good to optimize better: transparent cross-compartment wrappers. Things like bug 774119 are a result of us having a slow path there. I'm not saying we should fix CrossCompartmentWrapper in this bug (and in fact, I don't think we should), but we should keep the idea that we want to add it in mind when we do the refactoring in this bug.

Initial refactor with no additional functionality. It should be easy to add new proxy types now, though.

Comment on attachment 782749 [details] [diff] [review] Initial Refactor WIP Review of attachment 782749 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1420,5 @@ > + returnAddr); > + default: > + MOZ_ASSUME_UNREACHABLE("Bad GetPropKind"); > + } > +} Can this be refactored to something like GetPropertyIC::canAttachNativeGetPropStub which returns, say, GetPropertyIC::CanAttach{ReadSlot,CallGetter,None}? Then we can do away with passing in a pointer to &stubAttached and have control flow closer to GetElementIC's. @@ +1455,5 @@ > +TryAttachProxyGetPropStub(JSContext *cx, IonScript *ion, > + GetPropertyIC &cache, HandleObject obj, > + HandlePropertyName name, > + const SafepointIndex *safepointIndex, > + void *returnAddr, bool *stubAttached) Is there difficulty in also splitting this "Try" function into a "canAttach" and an "attach" pair? So that GetPropertyIC::update can be just an easy-to-read list of if (!attachedStub && cache.canAttachX(...)) { attachedStub = true; if (!cache.attachX(...)) return false; }

(In reply to Shu-yu Guo [:shu] from comment #3) > Comment on attachment 782749 [details] [diff] [review] > Initial Refactor WIP > > Review of attachment 782749 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/ion/IonCaches.cpp > @@ +1420,5 @@ > > + returnAddr); > > + default: > > + MOZ_ASSUME_UNREACHABLE("Bad GetPropKind"); > > + } > > +} > > Can this be refactored to something like > GetPropertyIC::canAttachNativeGetPropStub which returns, say, > GetPropertyIC::CanAttach{ReadSlot,CallGetter,None}? > > Then we can do away with passing in a pointer to &stubAttached and have > control flow closer to GetElementIC's. > That's a fine idea. Let's do that, or something close to it. The function is also called from the parallel IC's, so it'll be a little more complicated. See comment below. > @@ +1455,5 @@ > > +TryAttachProxyGetPropStub(JSContext *cx, IonScript *ion, > > + GetPropertyIC &cache, HandleObject obj, > > + HandlePropertyName name, > > + const SafepointIndex *safepointIndex, > > + void *returnAddr, bool *stubAttached) > > Is there difficulty in also splitting this "Try" function into a "canAttach" > and an "attach" pair? So that GetPropertyIC::update can be just an > easy-to-read list of > > if (!attachedStub && cache.canAttachX(...)) { > attachedStub = true; > if (!cache.attachX(...)) > return false; > } The trouble is that we want to eventually share much of the getprop code with getelem in the case where idval is a string. So this means that we are going to have to do some work to unify. I suppose this should be handled as a seperate bug once we have a cleaned up style. In general, I would like to remove all the static functions that aren't directly doing code generation, but it is going to take a bit of doing to fix up all the inbred codepaths as it seems amazingly silly to clean up everything as it stands, and then decide that we want to share the code again a bug later.

Refactor the proxy codepath to the side, as well as some general beautification. It is important to note the since we have removed the checkObj paths through the rest of the native code, we no longer need IsCacheableDOMProxy() checks in all our helpers to know about the prototype chain.

This was once half implemented as an attempt to combat lack of marking during GCs during OOL calls from ICs, but now, since we push the live registers before making the calls, it is totally unused.

Comment on attachment 784182 [details] [diff] [review] Part 1: Refactor GetPropertyICs to isolate proxy code Review of attachment 784182 [details] [diff] [review]: ----------------------------------------------------------------- Sounds good, I was afraid by the duplication, but the code is way easier to read, and thus feel less error prone than the previous versions. The fact that attach guards are now part of the function which are generating the code makes this code more secure and less error prone. Ideally, to be even less error prone, the tryAttach functions should look like: bool tryAttachFoo(…) { if (!checkCond1(…)) return true; if (!checkCond2(…)) return true; if (!collectData(…)) return true; *emitted = true; MacroAssembler masm(cx); if (!genCond1(…)) return false; if (!genCond2(…)) return false; return linkAndAttachStub(…); } but the current patch is a huge step forward. Thanks. ::: js/src/ion/IonCaches.cpp @@ +813,3 @@ > if (restoreScratch) > masm.pop(scratchReg); > + masm.bind(failures); failures might be NULL if there only one failure case. You should keep the past 3 lines conditionally executed only if we expect multiple failures. Label's assertions will ensure that all declared labels are bound, so you might still want to keep the prototype failure. @@ +1162,5 @@ > + const char *attachKind; > + > + switch (type) { > + case CanAttachReadSlot: > + GenerateReadSlot(cx, ion, masm, attacher, obj, name, holder, style nit: switch case indentation is 2/2: code; switch (type) { case CanAttachReadSlot: if (condition) { code; code; } @@ +1367,5 @@ > + if (canCache == CanAttachError) > + return false; > + if (canCache == CanAttachNone) > + return true; > + // Dont cache no properties, because the proxy can still contain the property nit: Dont -> Do not @@ +1395,5 @@ > + Address(object(), JSObject::offsetOfShape()), > + ImmGCPtr(obj->lastProperty()), > + &failures); > + > + // Make sure object is a DOMProxy proxy nit: remove the second proxy. @@ +1411,5 @@ > + Address(holderReg, JSObject::offsetOfShape()), > + ImmGCPtr(holder->lastProperty()), > + &failures); > + > + if (canCache == CanAttachReadSlot) I would prefer if we can assert that the inputs of canCache, as it is used at multiple locations. JS_ASSERT(canCache == CanAttachReadSlot || canCache == CanAttachCallGetter); such as we leave away the CanAttachArrayLength case which does not seems to be handled below. @@ +1543,1 @@ > AutoDetectInvalidation adi(cx, vp.address(), ion); Keep the comment above AutoDetectInvalidation. @@ +1553,3 @@ > if (cache.canAttachStub()) { > + if (!cache.tryAttachArgumentsLength(cx, ion, obj, name, &attachedStub)) > + return false; You should move all of these try functions into a tryAttachStub function of this IC. if (cache.canAttachStub() && !cache.tryAttachStub()) return false; Also attachedStub is not a good name, because there is a corner case where we might run out-of-memory in linkAndAttachStub, which might still return true even if we cannot link and attach the stub. So emitted makes more sense knowing that we do not garantee that a stub has been attached at the end. And even if the boolean is garanteed to be true, I would prefer to keep some symmetry between all the try calls. So I suggest to make the first one looks like: if (!attachedStub && tryAttachArgumentsLength(…)) return false; @@ +1699,5 @@ > + RootedScript script(cx); > + jsbytecode *pc; > + cache.getScriptedLocation(&script, &pc); > + return (IsCacheableGetPropReadSlot(obj, holder, shape) || > + IsCacheableNoProperty(obj, holder, shape, pc, output)); If shu agree, I think we should rename this function canAttachPureNative, and make it return a NativeGetPropCacheability, such as it is clear that this is a copy of canAttachNative where the major difference is the lookProperty function which is used. I do not want this function to become a near clone of canAttachNative without sharing common parts, as ICs are error prone.

(In reply to Nicolas B. Pierron [:nbp] from comment #7) > Comment on attachment 784182 [details] [diff] [review] > Part 1: Refactor GetPropertyICs to isolate proxy code > > Review of attachment 784182 [details] [diff] [review]: > ----------------------------------------------------------------- > > Sounds good, I was afraid by the duplication, but the code is way easier to > read, and thus feel less error prone than the previous versions. > The fact that attach guards are now part of the function which are > generating the code makes this code more secure and less error prone. > > Ideally, to be even less error prone, the tryAttach functions should look > like: > > bool tryAttachFoo(…) > { > if (!checkCond1(…)) > return true; > > if (!checkCond2(…)) > return true; > > if (!collectData(…)) > return true; > > *emitted = true; > MacroAssembler masm(cx); > > if (!genCond1(…)) > return false; > > if (!genCond2(…)) > return false; > > return linkAndAttachStub(…); > } > > but the current patch is a huge step forward. > Thanks. > I'm not sure I understand how this is materially different from what's in the patch. Can you clarify? We discussed moving many of the assertions to conditions instead, and that's more than fine. Is this what you mean? > ::: js/src/ion/IonCaches.cpp > @@ +813,3 @@ > > if (restoreScratch) > > masm.pop(scratchReg); > > + masm.bind(failures); > > failures might be NULL if there only one failure case. You should keep the > past 3 lines conditionally executed only if we expect multiple failures. > Label's assertions will ensure that all declared labels are bound, so you > might still want to keep the prototype failure. > I disagree. We return immediately above if !multipleFailureJumps, no? Would an assert before this make you feel better?

> @@ +1699,5 @@ > > + RootedScript script(cx); > > + jsbytecode *pc; > > + cache.getScriptedLocation(&script, &pc); > > + return (IsCacheableGetPropReadSlot(obj, holder, shape) || > > + IsCacheableNoProperty(obj, holder, shape, pc, output)); > > If shu agree, I think we should rename this function canAttachPureNative, > and make it return a NativeGetPropCacheability, such as it is clear that > this is a copy of canAttachNative where the major difference is the > lookProperty function which is used. > > I do not want this function to become a near clone of canAttachNative > without sharing common parts, as ICs are error prone. Setting needinfo, though probably agreement should be expressed in the form of f+?

(In reply to Eric Faust [:efaust] from comment #8) > (In reply to Nicolas B. Pierron [:nbp] from comment #7) > > bool tryAttachFoo(…) > > { > > if (!checkCond1(…)) > > return true; > > > > if (!checkCond2(…)) > > return true; > > > > if (!collectData(…)) > > return true; > > > > *emitted = true; > > MacroAssembler masm(cx); > > > > if (!genCond1(…)) > > return false; > > > > if (!genCond2(…)) > > return false; > > > > return linkAndAttachStub(…); > > } > > I'm not sure I understand how this is materially different from what's in > the patch. Can you clarify? We discussed moving many of the assertions to > conditions instead, and that's more than fine. Is this what you mean? The difference is that you have a clear symmetry which is explicit with the naming of checkFunction vs. genFunction, and even if these are tiny inlined functions, it makes things cleaner. Even somebody who don't know anything about the code would be able to spot a missing check/gen function, because of the symmetry. > > ::: js/src/ion/IonCaches.cpp > > @@ +813,3 @@ > > > if (restoreScratch) > > > masm.pop(scratchReg); > > > + masm.bind(failures); > > > > failures might be NULL if there only one failure case. You should keep the > > past 3 lines conditionally executed only if we expect multiple failures. > > Label's assertions will ensure that all declared labels are bound, so you > > might still want to keep the prototype failure. > > > > I disagree. We return immediately above if !multipleFailureJumps, no? Would > an assert before this make you feel better? You are right, I miss that part in the code.

Comment on attachment 784182 [details] [diff] [review] Part 1: Refactor GetPropertyICs to isolate proxy code Review of attachment 784182 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay. ::: js/src/ion/IonCaches.cpp @@ +1699,5 @@ > + RootedScript script(cx); > + jsbytecode *pc; > + cache.getScriptedLocation(&script, &pc); > + return (IsCacheableGetPropReadSlot(obj, holder, shape) || > + IsCacheableNoProperty(obj, holder, shape, pc, output)); There are other differences: - idempotency doesn't matter - I'd like the cx argument to be guaranteed locked. - we don't allow getters in parallel but still specialize Array.length How will code between the two versions be shared? I'm for sharing -- though I'd like to see how complex the combined function looks first. Last time I tried it looked too complicated to be worth getting rid of two easy-to-read versions without complex conditionals. Though now, the refactored canAttachNative does look much cleaner and is very similar to this function. If you come up with a refactoring, just r? me.

Here's only half a patch that tries to go back to a DetermineGetPropKind() sort of model, except with a templated version to handle specifics and ensure a locked context in the parallel case. Frankly, I think this constitutes a step in the *wrong* direction: The code is, to my estimate, much less readable than it used to be; we are back to hordes of little boolean functions flying around; and we have had to do some rather unpleasant surgery on the array length logic to accomodate the parallel ICs. I'll finish it if people insist, but I think we have now intertwined two codepaths into an over-branched monster where once there were two reasonably straightforward ways. I am not convinced that the net effect is towards obvious code correctness.

Comment on attachment 786595 [details] [diff] [review] Some attempt to unify canAttachNative Review of attachment 786595 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1100,5 @@ > + // If the cache is idempotenti or parallel, watch out for resolve hooks or > + // non-native objects on the proto chain. We check this before calling > + // lookupProperty, to make sure no effectful lookup hooks or resolve hooks > + // are called. > + if (cache.lookupNeedsIdempotentChain() && !obj->hasIdempotentProtoChain()) why just not calling it idempotent, aren't all Parallel IC supposed to be idempotent? @@ +1122,4 @@ > } > + return GetPropertyIC::CanAttachReadSlot; > + } else if (cache.allowArrayLength() && > + cache.canAttachArrayLength(cx, obj, name, holder, shape)) A Locked Context can be converted into a JSContext from what I understand, so I don't see the need ok keeping a Lock context if this is for duplicating functions.

(In reply to Nicolas B. Pierron [:nbp] from comment #13) > Comment on attachment 786595 [details] [diff] [review] > Some attempt to unify canAttachNative > > Review of attachment 786595 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/ion/IonCaches.cpp > @@ +1100,5 @@ > > + // If the cache is idempotenti or parallel, watch out for resolve hooks or > > + // non-native objects on the proto chain. We check this before calling > > + // lookupProperty, to make sure no effectful lookup hooks or resolve hooks > > + // are called. > > + if (cache.lookupNeedsIdempotentChain() && !obj->hasIdempotentProtoChain()) > > why just not calling it idempotent, aren't all Parallel IC supposed to be > idempotent? > You might think that, but their idempotency is unrelated to the state of the idempotent_ flag. It is expressly ignored, and its value just flaps in the breeze. > @@ +1122,4 @@ > > } > > + return GetPropertyIC::CanAttachReadSlot; > > + } else if (cache.allowArrayLength() && > > + cache.canAttachArrayLength(cx, obj, name, holder, shape)) > > A Locked Context can be converted into a JSContext from what I understand, > so I don't see the need ok keeping a Lock context if this is for duplicating > functions. Because typed arrays don't limit ArrayLength stubs to one static one.

Unification to come. The one comment was not altered, because it is utterly removed by another patch I'm about to post for review.

This whole stack is looking very green on try, along with the patches from bug 902264: https://tbpl.mozilla.org/?tree=Try&rev=045a22bc6b3c

Comment on attachment 787846 [details] [diff] [review] Fix nits from last review Review of attachment 787846 [details] [diff] [review]: ----------------------------------------------------------------- With the previous patch and this diff with the previous patch, this sounds fine. Fix the nits and add more comments to make it clear for the next readers. ::: js/src/ion/IonCaches.cpp @@ +1166,5 @@ > > switch (type) { > + case CanAttachReadSlot: > + GenerateReadSlot(cx, ion, masm, attacher, obj, name, holder, > + shape, object(), output()); nit: Fix indentation. @@ +1168,5 @@ > + case CanAttachReadSlot: > + GenerateReadSlot(cx, ion, masm, attacher, obj, name, holder, > + shape, object(), output()); > + attachKind = idempotent() ? "idempotent reading" > + : "non idempotent reading"; nit: same here. @@ +1420,5 @@ > } else { > // EmitGetterCall() expects |obj| to be the object the property is on to do some > // checks. Since we actually looked at checkObj, and no extra guards will be generated, > // we can just pass that instead. > + JS_ASSERT_IF(canCache != CanAttachCallGetter, canCache == CanAttachArrayLength); nit: Add a comment for this assertion to remind that Array.length is still a getter even if we optimize it most of the time. @@ +1563,5 @@ > + } > + > + if (!*emitted && !tryAttachTypedArrayLength(cx, ion, obj, > + name, emitted)) { > + return false; nit: In IonMonkey we wrap at 100 and not 80., you can keep these 2 on the same line and remove the brackets. ::: js/src/ion/IonCaches.h @@ +567,5 @@ > MutableHandleObject holder, > MutableHandleShape shape); > > + // Attach the proper stub, if possible > + bool tryAttachStub(JSContext *cx, IonScript *ion, HandleObject obj, nit: Improve the comment such as: Attach the stub if possible, returns false if an exception happened and set the emitted flag to true if we successfully generated code for the stub.

Comment on attachment 784182 [details] [diff] [review] Part 1: Refactor GetPropertyICs to isolate proxy code Review of attachment 784182 [details] [diff] [review]: ----------------------------------------------------------------- r+ this patch if folded with the nits patch that's I just r+.

Comment on attachment 784182 [details] [diff] [review] Part 1: Refactor GetPropertyICs to isolate proxy code Review of attachment 784182 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1119,5 @@ > + return CanAttachNone; > + } > + return CanAttachReadSlot; > + } > + else if (allowGetters() && (IsCacheableGetPropCallNative(obj, holder, shape) || No else-after-return, please @@ +1367,5 @@ > + if (canCache == CanAttachError) > + return false; > + if (canCache == CanAttachNone) > + return true; > + // Dont cache no properties, because the proxy can still contain the property Quoting "no properties" would make this clearer to me. @@ +1381,5 @@ > + // (if there is one). The generation is a constant in the generated > + // code and we will not have the same generation again for this > + // object, so the generation check in the existing IC would always > + // fail anyway. > + reset(); {} around multi-line if, even if there's only one line of code

Comment on attachment 787847 [details] [diff] [review] Part 3: Factor out Proxy::Get call generation Review of attachment 787847 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1262,5 @@ > + RootedId propId(cx, AtomToId(name)); > + masm.Push(propId, scratch); > + masm.movePtr(StackPointer, argIdReg); > + > + // Pushing object and receiver. Both are same, so Handle to one is equivalent to "Both are the same"?

Comment on attachment 787848 [details] [diff] [review] Part 4: Allow direct Proxy::Get call on unfound properties on DoesntShadow proxies Review of attachment 787848 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ -1380,5 @@ > if (canCache == CanAttachError) > return false; > if (canCache == CanAttachNone) > return true; > - // Dont cache no properties, because the proxy can still contain the property Oh, if you're removing this again, feel free to ignore my earlier nit @@ +1408,5 @@ > // Make sure object is a DOMProxy proxy > GenerateDOMProxyChecks(cx, masm, obj, name, object(), &failures); > > + if (holder) { > + // Found the propert on the prototype chain. Treat it like a native "property" @@ +1419,5 @@ > + > + // Guard on the holder of the property > + masm.moveNurseryPtr(ImmMaybeNurseryPtr(holder), holderReg); > + masm.branchPtr(Assembler::NotEqual, > + Address(holderReg, JSObject::offsetOfShape()), Indentation is off

Comment on attachment 787851 [details] [diff] [review] Unification Part 1: Fix GetPropertyIC::canAttachNative for unification Review of attachment 787851 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1150,5 @@ > +} > + > +/* static */ bool > +SequentialGetCache::doPropertyLookup(Context cx, HandleObject obj, HandlePropertyName name, > + MutableHandleObject holder, MutableHandleShape shape) Indentation seems off ::: js/src/ion/IonCaches.h @@ +790,5 @@ > > +class SequentialGetCache > +{ > + private: > + IonCache & cache_; & to one side? @@ +793,5 @@ > + private: > + IonCache & cache_; > + public: > + SequentialGetCache(IonCache &in) > + : cache_(in) I think the : is indented one more space

Comment on attachment 787855 [details] [diff] [review] Unification Part 2: Fix GetPropertyParIC for generalized CanAttachNative Review of attachment 787855 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.h @@ +1068,5 @@ > + private: > + const IonCache &cache_; > + public: > + ParallelGetCache(const IonCache &in) > + : cache_(in) As before

Comment on attachment 787857 [details] [diff] [review] Unification Part 3: Fix GetElementIC for unified CanAttachNative Review of attachment 787857 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.h @@ +801,5 @@ > public: > SequentialGetCache(const IonCache &in) > : cache_(in) > { > + JS_ASSERT_IF(!in.isGetProperty(), in.isGetElement()); I think this might be better as JS_ASSERT(in.isGetProperty() || in.isGetElement()); @@ +818,4 @@ > bool canMonitorSingletonUndefinedSlot(HandleObject holder, HandleShape shape) const { > + if (cache_.isGetProperty()) > + return cache_.toGetProperty().canMonitorSingletonUndefinedSlot(holder, shape); > + else Avoid else-after-return, probably with ?: here

Comment on attachment 787859 [details] [diff] [review] Unification Part 4: Fix GetElementParIC for unified CanAttachNative Review of attachment 787859 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.h @@ +1085,5 @@ > public: > ParallelGetCache(const IonCache &in) > : cache_(in) > { > + JS_ASSERT_IF(!in.isGetPropertyPar(), in.isGetElementPar()); Ditto @@ +1096,5 @@ > bool allowGetters() const { return false; } > TypedOrValueRegister output() const { > + if (cache_.isGetPropertyPar()) > + return cache_.toGetPropertyPar().output(); > + else Ditto

Comment on attachment 787849 [details] [diff] [review] Part 5: Add Generic Proxy::Get stub to GetPropertyIC Review of attachment 787849 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1508,5 @@ > + masm.setFramePushed(ion->frameSize()); > + > + // The branching around here is a little kludgy. It seems mostly unavoidable. > + > + // Ensure that the incoming object has one of the magic class pointers. Document the logical check happening here, e.g.: // Check that |object()| is an ObjectProxy, FunctionProxy, or OuterWindowProxy

Comment on attachment 787851 [details] [diff] [review] Unification Part 1: Fix GetPropertyIC::canAttachNative for unification Review of attachment 787851 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +1091,5 @@ > +template <class GetPropCache> > +static GetPropertyIC::NativeGetPropCacheability > +CanAttachNativeGetProp(const GetPropCache &cache, typename GetPropCache::Context cx, > + HandleObject obj, HandlePropertyName name, > + MutableHandleObject holder, MutableHandleShape shape) Nit: little weird to have a context parameter as the 2nd instead of the 1st parameter. @@ +1136,1 @@ > } SM style is no else after return. @@ +1166,5 @@ > RootedShape shape(cx); > RootedObject holder(cx); > > + NativeGetPropCacheability type = > + CanAttachNativeGetProp<SequentialGetCache>(*this, cx, obj, name, &holder, &shape); I'd rather not have the implicit conversion on |*this|. See the comment about not having the helper classes. ::: js/src/ion/IonCaches.h @@ +815,5 @@ > + } > + TypedOrValueRegister output() const { > + return cache_.toGetProperty().output(); > + } > +}; Do we need this extra class for the purpose of holding some typedefs and some functions which just pass through to the underlying cache? Can't we just have these in the GetPropertyIC class itself?

Comment on attachment 787855 [details] [diff] [review] Unification Part 2: Fix GetPropertyParIC for generalized CanAttachNative Review of attachment 787855 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.h @@ +1088,5 @@ > + > + static bool doPropertyLookup(Context cx, HandleObject obj, HandlePropertyName name, > + MutableHandleObject holder, MutableHandleShape shape); > +}; > + Same concern as part 1 on the necessity of this class.

Comment on attachment 787859 [details] [diff] [review] Unification Part 4: Fix GetElementParIC for unified CanAttachNative Review of attachment 787859 [details] [diff] [review]: ----------------------------------------------------------------- In general these patches look good; I'd rather not have 2 more classes to maintain which don't really stand on their own.

Comment on attachment 788435 [details] [diff] [review] Unification Part 1: Fix GetPropertyIC::canAttachNative for unification Review of attachment 788435 [details] [diff] [review]: ----------------------------------------------------------------- Look good! ::: js/src/ion/IonCaches.cpp @@ +1091,5 @@ > +template <class GetPropCache> > +static GetPropertyIC::NativeGetPropCacheability > +CanAttachNativeGetProp(const GetPropCache &cache, typename GetPropCache::Context cx, > + HandleObject obj, HandlePropertyName name, > + MutableHandleObject holder, MutableHandleShape shape) Nit: any reason the cx comes second?

Comment on attachment 788436 [details] [diff] [review] Unification Part 2: Fix GetPropertyParIC for generalized CanAttachNative Review of attachment 788436 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.h @@ +984,5 @@ > + } > + bool lookupNeedsIdempotentChain() const { return true; } > + bool canMonitorSingletonUndefinedSlot(HandleObject, HandleShape) const { return true; } > + bool allowGetters() const { return false; } > + Nit: trailing whitespace.

Comment on attachment 788438 [details] [diff] [review] Unification Part 4: Fix GetElementParIC for unified CanAttachNative Review of attachment 788438 [details] [diff] [review]: ----------------------------------------------------------------- Nice work, thanks for the cleanup. ::: js/src/ion/IonCaches.h @@ +1055,5 @@ > + } > + bool lookupNeedsIdempotentChain() const { return true; } > + bool canMonitorSingletonUndefinedSlot(HandleObject, HandleShape) const { return true; } > + bool allowGetters() const { return false; } > + Nit: trailing whitespace.

https://hg.mozilla.org/integration/mozilla-inbound/rev/954b005832ac https://hg.mozilla.org/integration/mozilla-inbound/rev/0768aa162e98 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5b43513cfc https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd1e46a79eb https://hg.mozilla.org/integration/mozilla-inbound/rev/f2f1ee68a475 https://hg.mozilla.org/integration/mozilla-inbound/rev/d04219b09245 https://hg.mozilla.org/integration/mozilla-inbound/rev/c81045074b71 https://hg.mozilla.org/integration/mozilla-inbound/rev/7017a8081582 https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7acdc4f30c https://hg.mozilla.org/integration/mozilla-inbound/rev/29e8662d6d81 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb1961adc27

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4fcb0471e0c9 - they took a little while to show up, but primarily in test_richtext2.html (mochitest-4 on debug 10.7 Mac, mochitest-8 on Android) we were getting GC crashes like https://tbpl.mozilla.org/php/getParsedLog.php?id=26419479&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=26420677&tree=Mozilla-Inbound, along with a few others like https://tbpl.mozilla.org/php/getParsedLog.php?id=26421132&tree=Mozilla-Inbound and https://tbpl.mozilla.org/php/getParsedLog.php?id=26419669&tree=Mozilla-Inbound which I presume are the same thing.

https://hg.mozilla.org/mozilla-central/rev/954b005832ac https://hg.mozilla.org/mozilla-central/rev/0768aa162e98 https://hg.mozilla.org/mozilla-central/rev/9d5b43513cfc https://hg.mozilla.org/mozilla-central/rev/0dd1e46a79eb https://hg.mozilla.org/mozilla-central/rev/f2f1ee68a475 https://hg.mozilla.org/mozilla-central/rev/d04219b09245

This should fix the bustage that was seen on inbound. I will fold these into the correct patches in the stack before pushing, but they are shown here as one patch for ease of review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/958f4d97203b https://hg.mozilla.org/integration/mozilla-inbound/rev/328ba3501b3d https://hg.mozilla.org/integration/mozilla-inbound/rev/15759c4e73e4 https://hg.mozilla.org/integration/mozilla-inbound/rev/5315e1c501b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/be2e15a98350 https://hg.mozilla.org/integration/mozilla-inbound/rev/9300e9961070 https://hg.mozilla.org/integration/mozilla-inbound/rev/2952d7a31a23 https://hg.mozilla.org/integration/mozilla-inbound/rev/24e5fd5b257e https://hg.mozilla.org/integration/mozilla-inbound/rev/74ace791d9a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/38ee8edde54d https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffe6d8e4328

https://hg.mozilla.org/mozilla-central/rev/958f4d97203b https://hg.mozilla.org/mozilla-central/rev/328ba3501b3d https://hg.mozilla.org/mozilla-central/rev/15759c4e73e4 https://hg.mozilla.org/mozilla-central/rev/5315e1c501b7 https://hg.mozilla.org/mozilla-central/rev/be2e15a98350 https://hg.mozilla.org/mozilla-central/rev/9300e9961070 https://hg.mozilla.org/mozilla-central/rev/2952d7a31a23 https://hg.mozilla.org/mozilla-central/rev/24e5fd5b257e https://hg.mozilla.org/mozilla-central/rev/74ace791d9a2 https://hg.mozilla.org/mozilla-central/rev/38ee8edde54d https://hg.mozilla.org/mozilla-central/rev/9ffe6d8e4328