Closed Bug 797977 Opened 12 years ago Closed 12 years ago

IonMonkey: Enable StackIter::fp() assertion.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 --- unaffected
firefox18 --- affected
firefox19 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: nbp, Assigned: nbp)

References

Details

(Keywords: assertion, Whiteboard: [ion:p1][adv-main19+])

Attachments

(1 file, 3 obsolete files)

Currently there is a remaining comment on top of StackIter::fp() asking for adding !isIon() to the assertion, one use case is comming from fun_getProperty and another is coming from pushExecuteFrame (under DebuggerFrame_eval).

I am haven't investigated all the risk yet, so I flagging this as s-s in the mean time and flag it for ff18.
Whiteboard: [ion:p1:fx18] → [ion:p1]
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Add extra assertions which should not break, and remove invalid use cases of StackIter::fp().
Attachment #668275 - Flags: review?(luke)
Comment on attachment 668275 [details] [diff] [review]
Embed !isIon() assertion in StackIter::fp().

Great!
Attachment #668275 - Flags: review?(luke) → review+
Ok, this is no longer the same size of patch, but this one brings more sanity and gave me the opportunity to review all uses cases.

jsdbgapi is remaining unchanged with one restriction which is that JSStackFrame returned by the JS_Broken* function are only non-Ion interpreter frames.
Attachment #668275 - Attachment is obsolete: true
Attachment #668731 - Flags: review?(luke)
Comment on attachment 668731 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

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

::: js/src/jsapi.cpp
@@ +7312,5 @@
>      ScriptFrameIter i(cx);
>      if (i.done())
>          return cx->global();
> +    RootedObject scopeChain(cx, i.scopeChain());
> +    return &scopeChain->global();

return &i.scopeChain()->global();

seems fine; global() is a simple loop.

::: js/src/jscntxtinlines.h
@@ -586,5 @@
>  }
>  
> -/* Get the current frame, first lazily instantiating stack frames if needed. */
> -static inline js::StackFrame *
> -js_GetTopStackFrame(JSContext *cx, FrameExpandKind expand)

Happy to see this old wart go.

::: js/src/jscompartment.cpp
@@ +672,5 @@
>  
>  bool
>  JSCompartment::hasScriptsOnStack()
>  {
> +    for (AllFramesIter f(rt->stackSpace); !f.done(); ++f) {

If you're going to make it longer, how about 'afi'?

@@ +678,5 @@
> +        // If this is an Ion frame, check the IonActivation instead
> +        if (f.fp()->beginsIonActivation())
> +            continue;
> +#endif
> +        if (f.fp()->script()->compartment() == this)

This code makes me think we need to rename AllFramesIter::fp to AllFramesIter::interpFrame and add AllFramesIter::isIon.

::: js/src/jsdbgapi.cpp
@@ +482,5 @@
> +        js::mjit::ExpandInlineFrames(cx->compartment);
> +#endif
> +        fp = cx->maybefp();
> +    } else
> +        fp = fp->prev();

{ } since the then-branch has 'em.

::: js/src/jsobj.cpp
@@ +4449,5 @@
>  JS_FRIEND_API(bool)
>  js::CheckUndeclaredVarAssignment(JSContext *cx, JSString *propname)
>  {
> +    JSScript *script = cx->stack.currentScript();
> +    if (!script)

I think you want currentScript(NULL, ALLOW_CROSS_COMPARTMENT).

::: js/src/shell/js.cpp
@@ +1383,5 @@
>      JS_ASSERT(!iter.done());
>  
> +    // interpFrame might assert that we are not using an IonMonkey
> +    // fake-interpreter frame. This would not be fix unless we want to support
> +    // IonMonkey frames in JS_EvaluateUCInStackFrame.

This comment is confusing.  How about "Debug-mode currently disables Ion compilation."

@@ +2508,5 @@
>      JS_ASSERT(cx->hasfp());
>  
> +    // interpFrame might assert that we are not using an IonMonkey
> +    // fake-interpreter frame. This would not be fix unless we want to support
> +    // IonMonkey frames in JS_EvaluateUCInStackFrame.

ditto

::: js/src/vm/Stack.cpp
@@ +1055,5 @@
>      if (evalInFrame) {
>          /* Though the prev-frame is given, need to search for prev-call. */
>          StackSegment &seg = cx->stack.space().containingSegment(evalInFrame);
>          StackIter iter(cx->runtime, seg);
> +        JS_ASSERT(!evalInFrame->beginsIonActivation());

Can you comment here "Debug-mode currently disables Ion compilation".  Then you should be able to take out the "|| iter.isIon".  That way we'll hit the assert eagerly if we ever add debug-mode instead and fix it correctly.

::: js/src/vm/Stack.h
@@ +1774,5 @@
> +    // the interpreter and by JagerMonkey. IonMonkey does not maintain this
> +    // interpreter frame but use one to indicate the presence of an activation
> +    // of IonMonkey code on the C stack. Such fake interpreter frame contains
> +    // mostly garbage, and so access to such frame is limited.
> +    StackFrame *interpFrame() const { JS_ASSERT(isScript() && !isIon()); return fp_; }

How about replacing the last two sentences with:
 "When entering IonMonkey, the top interpreter frame (pushed by the caller) is kept on the stack as bookkeeping (with beginsIonActivation() set).  The contents of the frame are ignored by Ion code (and GC) and thus immediately become garbage and must not be touched directly."
Attachment #668731 - Flags: review?(luke) → review+
Compared to the previous version:
- Apply nits.
- Fix StackIter::* for the native case (use fp_ instead of interpFrame())
- Fix assertions (beginsIonActivation -> runningInIon), see Bug 798792 comment 4.  And fixing Bug 798792 issue too.
- Add StackIter::compartment for checking that all debugged frames are not Ion frames.
Attachment #668731 - Attachment is obsolete: true
Attachment #669352 - Flags: review?(luke)
Comment on attachment 669352 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

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

::: js/src/jscompartment.cpp
@@ +676,5 @@
> +    for (AllFramesIter afi(rt->stackSpace); !afi.done(); ++afi) {
> +#ifdef JS_ION
> +        // If this is an Ion frame, check the IonActivation instead
> +        if (afi.fp()->runningInIon())
> +            continue;

Again, could you rename AllFramesIter::fp to interpFrame and add a AllFramesIter::inIon() that can be used for this branch?

::: js/src/vm/Stack.cpp
@@ +1530,5 @@
> +        break;
> +#endif
> +      case SCRIPTED:
> +      case NATIVE:
> +        return fp_->compartment();

NATIVE shouldn't use fp_ (there might not be one or it might be in a different compartment).  Use  calls_->callee().compartment() for the NATIVE case instead.
Attachment #669352 - Flags: review?(luke) → review+
Delta:
- Fix StackIter Native's compartment.
- Rename AllFrameIter::fp() to AllFrameITer::interpFrame().
- Filter out Ion frames from AllFrameIter iterations, because they are either used for debug or handled in other path (such as jscompartment.cpp & jsscript.cpp)

\o/ No more un-garded usage of “StackFrame *” which is an Ion activation frame.
Thanks for the renaming suggestion.
Attachment #669352 - Attachment is obsolete: true
Attachment #669861 - Flags: review?(luke)
Comment on attachment 669861 [details] [diff] [review]
Rename StackIter::fp() to StackIter::interpFrame().

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

Nice job wading through all this and fixing the next 5 fuzz bugs :)

::: js/src/ion/IonFrameIterator.h
@@ +301,5 @@
>          return si_;
>      }
>      bool isFunctionFrame() const;
>      bool isConstructing() const;
> +    JSObject *scopeChainObject() const;

scopeChain() would be shorter and more consistent.

::: js/src/jsscript.cpp
@@ +2572,5 @@
>       *  - type inference data for the script assuming script->needsArgsObj; and
>       */
>      for (AllFramesIter i(cx->stack.space()); !i.done(); ++i) {
> +        if (i.isIon())
> +            continue;

How about a comment on this to the effect of:

We cannot reliably create an arguments object for Ion activations of this script.  To maintain the invariant that "script->needsArgsObj implies fp->hasArgsObj", the Ion bail mechanism will create an arguments object right after restoring the StackFrame and before entering the interpreter.  This delay is safe since the engine avoids any observation of a StackFrame when it beginsIonActivation (see StackIter::interpFrame comment).

::: js/src/vm/Debugger.cpp
@@ +1761,5 @@
>       * cx->fp() would return the topmost frame in the current context.
>       * Since there may be multiple contexts, use AllFramesIter instead.
>       */
>      for (AllFramesIter i(cx->stack.space()); !i.done(); ++i) {
> +        /* Debug-mode currently disables Ion compilation. */

By itself, this comment would seem to indicate that we shouldn't find any i.isIon() frames.  Perhaps a slight expansion: "Debug-mode currently disables Ion compilation in the compartment of the debuggee." ?

::: js/src/vm/ScopeObject.cpp
@@ +1835,5 @@
>       */
>      for (AllFramesIter i(cx->runtime->stackSpace); !i.done(); ++i) {
> +        /* Debug-mode currently disables Ion compilation. */
> +        if (i.isIon())
> +            continue;

ditto

::: js/src/vm/Stack.cpp
@@ +825,5 @@
>  {
>      for (AllFramesIter i(*this); !i.done(); ++i) {
> +        /* Debug-mode currently disables Ion compilation. */
> +        if (i.isIon())
> +            continue;

ditto

::: js/src/vm/Stack.h
@@ -1527,5 @@
>      /* The StackSpace currently hosting this ContextStack. */
>      StackSpace &space() const { return *space_; }
>  
> -    /* Return whether the given frame is in this context's stack. */
> -    bool containsSlow(const StackFrame *target) const;

nice catch
Attachment #669861 - Flags: review?(luke) → review+
My dev-branch on try, before rebasing and fixing a minor merge issue between 2 of my patches (in js/src/vm/ArgumentsObject.cpp):
https://tbpl.mozilla.org/?tree=Try&rev=98d875f58193

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebeca12019a2
https://hg.mozilla.org/mozilla-central/rev/ebeca12019a2
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
What's the rating on this bug? Do we think that B2G is impacted?
(In reply to Alex Keybl [:akeybl] from comment #14)
> What's the rating on this bug? Do we think that B2G is impacted?

This patch should hopefully not change any behavior.  So I don't think is is needed for B2G.
Whiteboard: [ion:p1] → [ion:p1][adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.