Fennec 20 is going to suffer from that crash if we do not fix it.

Minimized test case.

That patch makes sure the two generic methods (::ConvertNumberToString() and ::ConvertStringToNumber()) use a JSAutoRequest and a safe js context (which should prevent Date.prototype bustage) in case of they are called without JS in the stack. ::GetValueAsDate() and ::SetValueAsDate() should not be called without JS in the stack. Note that ::ConvertNumberToString() will unlikely be called without JS in the stack but it is a generic-enough method that it might easily be used at places that could not have JS in their stack.

Comment on attachment 699901 [details] [diff] [review] Patch Review of attachment 699901 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsHTMLInputElement.cpp @@ +1078,5 @@ > return true; > } > case NS_FORM_INPUT_DATE: > { > + JSContext* ctx = nsContentUtils::GetSafeJSContext(); So, unconditionally using the safe JS context like this is kind of sketchy. The issue is that, if the JS can trigger arbitrary script, we might re-enter gecko on the safe JS context with some other cx being on the JSContext stack. I've seen this handled in lots of different ways, including: 1 - Checking GetCurrentJSContext, and only using the SafeJSContext if it's null. 2 - Pushing the Safe JSContext, and using it. 3 - Doing 1, but also pushing the SafeJSContext. The eventual goal here is to have only one JSContext for Gecko, and indicate system actors by some mechanism other than pushing a null cx. In that world, this isn't a problem. So the question is, what's the best thing to do in the mean time? If we do (1), I think we should add an nsContentUtils::GetCurrentOrSafeJSContext(). If we do (2) or (3), I think we should add an RAII nsAutoJSContext to nsContentUtils that automatically does the right thing. I'm going to flag Blake and Boris as needinfo here. @@ +1083,3 @@ > if (!ctx) { > return false; > } IMO you could get rid of this check. The browser is unlikely to get very far without a working SafeJSContext. But up to you.

Flagging bz per comment 4.

> The issue is that, if the JS can trigger arbitrary script So the reason I suggested Mounir use that is actually because that also gives him a safe global, as far as I know. In particular, one in which no one has messed with Date.prototype. And given that, there should be no arbitrary script running: we're just making calls to Date.prototype methods. Is my understanding that the compartment on the safe context comes along with a safe global correct? If it is, then there isn't a problem. If, not, there is no point to using the safe context here and we can keep using the document's context but have to do all the termination func holder and nsCxPusher jazz.

(In reply to Boris Zbarsky (:bz) from comment #6) > > The issue is that, if the JS can trigger arbitrary script > > So the reason I suggested Mounir use that is actually because that also > gives him a safe global, as far as I know. In particular, one in which no > one has messed with Date.prototype. And given that, there should be no > arbitrary script running: we're just making calls to Date.prototype methods. > Is my understanding that the compartment on the safe context comes along > with a safe global correct? "Probably". The only concern is that somebody else might have pushed and used the safe JS context above us, and entered a compartment somewhere. I know that workers, at least, push the safe JSContext and use it. The problem there is that you need to push the context _again_ in order to trigger JS_SaveFrameChain to hide the previous history and temporarily stick you back in the default compartment. It should be pretty easy. Basically, just make an NS_STACK_CLASS in nsContentUtils.h something like this: NS_STACK_CLASS nsAutoJSContext { private: JSContext *mCx; Maybe<nsCxPusher> mPusher; public: nsAutoJSContext() : mCx(nullptr) {}; bool init() { mCx = nsContentUtils::GetCurrentJSContext(); if (!mCx) { mCx = nsContentUtils::GetSafeJSContext(); mPusher.Construct(); if (!mPusher->Push(mCx)) { return false; } } return true; } // some helpers to coerce this function to an JSContext* and &. } We can then use this here, and also rip out AutoSafeJSContext in workers and use it there as well. How does that sound?

Actually, given that nsCxPusher already does minimal work on construction (using .Push() instead), the Maybe<> is probably unnecessary.

> The problem there is that you need to push the context _again_ in order to trigger > JS_SaveFrameChain to hide the previous history Gah. At that point you might as well just use the page context, no? > Basically, just make an NS_STACK_CLASS in nsContentUtils.h Making it easier to use the safe JS context is a non-goal, imo. We just need something in this bug to fix things temporarily until we can move away from needing a jscontext here at all.

(In reply to Boris Zbarsky (:bz) from comment #9) > > The problem there is that you need to push the context _again_ in order to trigger > > JS_SaveFrameChain to hide the previous history > > Gah. At that point you might as well just use the page context, no? You still have the handle the case where the page context isn't the top context on the stack. Executing script when that invariant is violated is a bug AFAICT. > Making it easier to use the safe JS context is a non-goal, imo. We just > need something in this bug to fix things temporarily until we can move away > from needing a jscontext here at all. Yes, but that's at least 6 months out. In the mean time, I would prefer to just write this easy piece of machinery so that we don't have to revisit this the next time. This doesn't complicate our lives when we single-cx the DOM. It actually makes it easier, because we know that when people use an nsAutoJSContext, they just want whatever context is active, which, in the endgame, will be the only context in existence.

> You still have the handle the case where the page context isn't the top context on the > stack To be clear, "use the page context" means nsCxPusher and a termination func holder, since that's the only way to call script on it safely. I'd been hoping we could avoid some of that complexity with the safe context, but if not there's no point using it. > Yes, but that's at least 6 months out. Why? Do you know something about schedules I don't? All we need here is to convert input to WebIDL (part of this quarter's goals) and expose jsapi for the date manipulation that doesn't need a JSContext (whenever someone sits down and does it). Or am I missing something? If you were talking about the single-jscontetx patches, we don't need those here to avoid using a JSContext; we just need to stop doing manual jsapi crap. ;)

(In reply to Boris Zbarsky (:bz) from comment #11) > > You still have the handle the case where the page context isn't the top context on the > > stack > > To be clear, "use the page context" means nsCxPusher and a termination func > holder, since that's the only way to call script on it safely. I'd been > hoping we could avoid some of that complexity with the safe context, but if > not there's no point using it. Yes, and I'm saying we should bundle this all up into a helper in nsContentUtils that does the right thing in all cases. > Why? Do you know something about schedules I don't? All we need here is to > convert input to WebIDL (part of this quarter's goals) and expose jsapi for > the date manipulation that doesn't need a JSContext (whenever someone sits > down and does it). Or am I missing something? If you were talking about > the single-jscontetx patches, we don't need those here to avoid using a > JSContext; we just need to stop doing manual jsapi crap. ;) I was referring to the latter, yes. But this pattern exists in lots and lots of places, not just here. I've seen it bamboozle enough people that I'd like to solve it properly.

> and I'm saying we should bundle this all up into a helper in nsContentUtils that does the > right thing in all cases. OK, fair.

What about that?

I prefer to do the worker change in another bug/patch in case of the workers have needs that nsHTLMInputElement do not.

This is differentiating the SafeAutoJSContext and AutoJSContext. I used inheritance but because the main difference is in the ctor, it isn't very nice and unfortunately, it is way too early to use the C++11 delegating ctor feature :(

Comment on attachment 701225 [details] [diff] [review] Patch v3 Please throw in MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER and MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM or MOZ_GUARD_OBJECT_NOTIFIER_PARAM as needed.

Comment on attachment 701225 [details] [diff] [review] Patch v3 Review of attachment 701225 [details] [diff] [review]: ----------------------------------------------------------------- r=bholley with comments addressed. ::: content/base/public/nsContentUtils.h @@ +2291,5 @@ > }; > > +namespace mozilla { > + > +class NS_STACK_CLASS AutoJSContext { Please include some verbose documentation for both these classes. @@ +2305,5 @@ > + // supported to be able to compile Gecko. > + void Init(bool aSafe); > + > + JSContext* mCx; > + Maybe<nsCxPusher> mPusher; Why do we need the Maybe<> here? nsCxPusher's constructor does almost nothing.

(In reply to Bobby Holley (:bholley) from comment #18) > @@ +2305,5 @@ > > + // supported to be able to compile Gecko. > > + void Init(bool aSafe); > > + > > + JSContext* mCx; > > + Maybe<nsCxPusher> mPusher; > > Why do we need the Maybe<> here? nsCxPusher's constructor does almost > nothing. Actually, you used it in comment 7, that is why I used it (never hard about Maybe<> before). I agree that we could just live without.

(In reply to Mounir Lamouri (:mounir) from comment #19) > (In reply to Bobby Holley (:bholley) from comment #18) > > @@ +2305,5 @@ > > > + // supported to be able to compile Gecko. > > > + void Init(bool aSafe); > > > + > > > + JSContext* mCx; > > > + Maybe<nsCxPusher> mPusher; > > > > Why do we need the Maybe<> here? nsCxPusher's constructor does almost > > nothing. > > Actually, you used it in comment 7 Yes, but I retracted that in comment 8. ;-)

Why does dom.experimental_forms need to remain enabled for mobile in Firefox 20?

(In reply to Alex Keybl [:akeybl] from comment #21) > Why does dom.experimental_forms need to remain enabled for mobile in Firefox > 20? "dom.experimental_forms" is enabled by default because the features behind that pref are actually ready to be used on Mobile but not on Desktop.

Unfortunately, with that patch, my crashtest fails: 0x00007ffff4538ce2 in JSRuntime::assertValidThread (this=0x7fffffffb1d0) at /home/mounir/Projects/mozilla/forms/js/src/jsapi.cpp:1080 1080 JS_ASSERT(ownerThread_ == PR_GetCurrentThread()); With this stack: #0 0x00007ffff4538ce2 in JSRuntime::assertValidThread (this=0x7fffffffb1d0) at /home/mounir/Projects/mozilla/forms/js/src/jsapi.cpp:1080 #1 0x00007ffff454a44b in StartRequest (cx=0x7fffffffb1a0) at /home/mounir/Projects/mozilla/forms/js/src/jsapi.cpp:1174 #2 0x00007ffff454a5b4 in JS_BeginRequest (cx=0x7fffffffb1a0) at /home/mounir/Projects/mozilla/forms/js/src/jsapi.cpp:1210 #3 0x00007ffff1d91e85 in JSAutoRequest::JSAutoRequest (this=0x7fffffffb1c0, cx=0x7fffffffb1a0, _notifier=...) at ../../../dist/include/jsapi.h:3056 #4 0x00007ffff2776c27 in nsHTMLInputElement::ConvertStringToNumber (this=0x7fffdaef6580, aValue=..., aResultValue=@0x7fffffffb2e8: 6.9533445886701833e-310) at /home/mounir/Projects/mozilla/forms/content/html/content/src/nsHTMLInputElement.cpp:1085 #5 0x00007ffff2776ece in nsHTMLInputElement::GetValueAsDouble (this=0x7fffdaef6580) at /home/mounir/Projects/mozilla/forms/content/html/content/src/nsHTMLInputElement.cpp:1137 Boris and Bobby, does one of you have an idea off hand?

Comment on attachment 702388 [details] [diff] [review] Patch v4 Yes. Your cx is garbage. You need to initialize it in the constructor.

(In reply to Mounir Lamouri (:mounir) from comment #22) > (In reply to Alex Keybl [:akeybl] from comment #21) > > Why does dom.experimental_forms need to remain enabled for mobile in Firefox > > 20? > > "dom.experimental_forms" is enabled by default because the features behind > that pref are actually ready to be used on Mobile but not on Desktop. We'll track for FF20 and take a forward fix when ready, unless we expect other regressions (in which case a justification should be made for why dom.experimental_forms can't first be enabled in FF21). Can we add the associated crash signature?

nsCOMPtr completely broke my mental representation of pointers and made me think that all pointers were initialized with 0. So sad...

Comment on attachment 704515 [details] [diff] [review] Patch v4.1 r=me

Comment on attachment 704515 [details] [diff] [review] Patch v4.1 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 769352 User impact if declined: crash when <input type='date'> is present in a page (in most cases) Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): very low risk given that it will only impact <input type='date'> and that would be its first release.

Comment on attachment 704515 [details] [diff] [review] Patch v4.1 Approving on aurora given "dom.experimental_forms" is enabled by default on mobile for FF20 which needs the low risk patch here.