1006291 - Need a way to insert some kind of activation marker that we can rewind back to

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

See the discussion in bug 961325 comment 57. Basically, when we instantiate an AutoEntryScript, we'd like to be able to get the file/line of the first bit of script that executes within the scope of an AutoEntryScript. This corresponds to the concrete "entry point", which we don't necessarily know when we instantiate the AutoEntryScript.

Luke, do you have any thoughts on how to design this?

Comment 1

[Luke Wagner [:luke]]

Does the AutoEntryScript know the script/function that is about to be run?  Given that, it seems like the AutoEntryScript can derive the first line of the function/script itself.

Comment 2

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #1)
> Does the AutoEntryScript know the script/function that is about to be run? 
> Given that, it seems like the AutoEntryScript can derive the first line of
> the function/script itself.

Not necessarily, no. But in the cases that it doesn't (often when it calls EvaluateString), it generally has access to the file/line out-of-band.

So maybe we should just do the following:

Give MozExecutionReason several constructors - one that takes a HandleScript, one that takes a HandleFunction, and one that takes an explicit file/line. Each one takes an nsCString for a "reason". It also has a separate SetEventType and whatnot.

Make AutoEntryScript take a mandatory MozExecutionReason&. This means that the usage pattern goes like this:

MozExecutionReason reason(NS_LITERAL_CSTRING("Some reason"), myScript);
AutoEntryScript aes(global, reason);

Panos, Bob - does that sound good?

Comment 3

[Boris Zbarsky [:bzbarsky]]

The AutoEntryScript for a WebIDL callback interface doesn't know the script/function it's about to run.  In fact, it will either end up doing a JS_GetProperty, or a JS_SetProperty (which can run script, but it doesn't know what) or a JS_GetProperty followed by a JS::Call if the value it got is callable.

Comment 4

[Boris Zbarsky [:bzbarsky]]

And even for a WebIDL callback, it has a callable object, but that may not be a Function, of course.

Comment 5

[Bob Owen (:bobowen) PTO until 8th Oct]

(In reply to Bobby Holley (:bholley) from comment #2)

> Make AutoEntryScript take a mandatory MozExecutionReason&. This means that
> the usage pattern goes like this:
> 
> MozExecutionReason reason(NS_LITERAL_CSTRING("Some reason"), myScript);
> AutoEntryScript aes(global, reason);
> 
> Panos, Bob - does that sound good?

After a quick bit of reading to catch up, that looks pretty straight forward.
The only bit I'm not totally clear on is how we'll always have something to pass for the second constructor parameter for MozExecutionReason ... for example the one you will be adding in nsGlobalWindow::SetNewDocument for Bug 997440.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #3)
> The AutoEntryScript for a WebIDL callback interface doesn't know the
> script/function it's about to run.  In fact, it will either end up doing a
> JS_GetProperty, or a JS_SetProperty (which can run script, but it doesn't
> know what) or a JS_GetProperty followed by a JS::Call if the value it got is
> callable.

Ah, good point.

(In reply to Bob Owen (:bobowen) from comment #5)
> for
> example the one you will be adding in nsGlobalWindow::SetNewDocument for Bug
> 997440.

And this (JS_FireOnNewGlobalObject). So yeah, suffice it to say that we don't always know the answer to this. Do you have other ideas, luke?

Comment 7

[Luke Wagner [:luke]]

By any chance does AutoEntryScript HideScriptedCaller?  If so, then we should be able to define an OutermostScriptedCaller that stopped at any activation that had been hidden.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #7)
> By any chance does AutoEntryScript HideScriptedCaller?  If so, then we
> should be able to define an OutermostScriptedCaller that stopped at any
> activation that had been hidden.

No, but AutoIncumbentScript does. Now, at present, AutoIncumbentScript is only ever instantiated immediately after an AutoEntryScript. So it's possibly that we could just merge them together. But this is not how the model in the spec works, so it'd be risky.

Comment 9

[Bobby Holley (:bholley)]

In effect, I think the safest thing is to have something analogous to HideScriptedCaller, but that users a different piece of state.

Comment 10

[Bobby Holley (:bholley)]

(JS::EnterScript/JS::LeaveScript)

Comment 11

[Luke Wagner [:luke]]

IIUC, whereas 'incumbent script' is a well-defined thing, the info we're trying to get here is for the debugger and so it's meaning is not exactly fixed.  Can we just use the incumbent script?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #11)
> IIUC, whereas 'incumbent script' is a well-defined thing, the info we're
> trying to get here is for the debugger and so it's meaning is not exactly
> fixed.  Can we just use the incumbent script?

No. The incumbent script is generally the result of JS::DescribeScriptedCaller, modulo any exceptions in which we create an AutoIncumbentScript (which instantiates an AutoHideScriptedCaller). The debugger wants the entry point, not the current stack frame.

Comment 13

[Luke Wagner [:luke]]

Ok, well I guess I mean the entry incumbent script; isn't there a stack that is maintained that you consult when DescribeScriptedCaller returns null?

Comment 14

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #13)
> Ok, well I guess I mean the entry incumbent script

That's not a thing. Entry points are when the UA enters script (event handlers, callbacks, <script elements> etc). The "incumbent script" is the scripted caller, modulo a few exceptions.

> ; isn't there a stack that
> is maintained that you consult when DescribeScriptedCaller returns null?

The Script Settings stack, yes. AutoEntryScript and AutoIncumbentScript each push an entry onto the script settings stack. The item pushed by AutoEntryScript is marked a "candidate entry point". When you try to get the Entry Point, you get the topmost item that is also a "candidate entry point" (skipping any higher-up entries that are not candidates).

The item pushed by AutoIncumbentScript is not marked as a "candidate entry point". AutoIncumbentScript is useful because it also instantiates an AutoHideScriptedCaller, which causes GetScriptedCallerGlobal to return null, allowing the embedding to fall back the topmost entry on the Script Settings Stack (irrespective of candidate-ness). This is all just a mechanism to get the effect of dummy frames without the negative consequences.

But none of that helps us here. The whole problem here is that items in the Script Settings Stack only track a global, not the file/line stuff that Panos needs (see comment 6).

Comment 15

[Luke Wagner [:luke]]

Ah, and, you're saying, when we push something onto the Script Settings stack, the reason we are about to run script is not yet known or otherwise "far away"?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Luke Wagner [:luke] (on partial PTO) from comment #15)
> Ah, and, you're saying, when we push something onto the Script Settings
> stack, the reason we are about to run script is not yet known or otherwise
> "far away"?

No, the reason is known. The file and line are not known, per the two examples from bz and bob in comment 6. That's why I originally filed this bug.

Comment 17

[Bobby Holley (:bholley)]

Ok, stepping back for a moment. This discussion began because jimb wanted to keep Saved Frame Chains around for uses like this one - see bug 979730 comment 12 and onward.

But Saved Frame Chains have all sorts of crazy properties within the JS engine that we want to get rid of. So I suggested we use the Script Setting Stack instead. The problem we're running into here is that, modulo Saved Frame Chains (which we don't want to add new dependencies for), we don't currently have a good way to go from a ScriptSettingStackEntry to the equivalent location on the JS stack.

So, in keeping with Luke's historical logic on this subject, I think we should introduce a piece of state that has a much more limited role, and hoists most of the heavy-lifting onto the embedding. It can work analogously to hideScriptedCallerCount_, and we can then instantiate a JS::AutoEntryPoint within AutoEntryScript, and add the appropriate machinery to retrieve the first scripted frame since the instantiation of the most recent JS::AutoEntryPoint.

Luke, how does that sound to you?

Comment 18

[Luke Wagner [:luke]]

(In reply to Bobby Holley (:bholley) from comment #17)
Yeah, that's where I figured where we were going since comment 9, I was just trying to ascertain if we *really* need another independent marker on the stack... but it sounds like we do, so sounds good to me.

Comment 19

[Bobby Holley (:bholley)]

Ok, cool. This patch should be simple to write, and either Panos or Jim should be able to whip it up in a snap. Let me know if you need any help.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

It would really help me if you could provide a more detailed breakdown of what needs to be done.

Comment 21

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #20)
> It would really help me if you could provide a more detailed breakdown of
> what needs to be done.

(1) Add a field to Activation called entryPoint_ similar to savedFrameChain_ and hideScriptedCallerCount_.
(2) Add the equivalent of JS::HideScriptedCaller, JS::UnhideScriptedCaller, and JS::AutoHideScriptedCaller.
(4) Make AutoEntryScript instantiate the RAII class from 2 (after the cx push).
(4) Add a JS::DescribeEntryPoint, similar to JS::DescribeScriptedCaller.

Comment 22

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v1

Thanks for the detailed instructions! Is this what you had in mind?

Comment 23

[Bobby Holley (:bholley)]

Comment on attachment 8421770 [details] [diff] [review]
v1

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

This isn't quite right. The whole "hide" and "override" metaphor doesn't really apply to case of entry points. I can go into more detail if need be, but I'm pretty busy right now. Can you talk with jimb about it?

Comment 24

[Panos Astithas (he/him) [:past] (please ni?)]

Working on the execution reasons patch I realized that it's not just the naming of things that is incorrect here. In the tests in bug 961325 I see that the entry point is being attributed to the debugger frames on the stack, instead of the debuggee frames. It looks like I would have to skip initialization of AutoEntryScript.mEntryPointOverride for debugger frames, but I don't know how to go about that. Jim, halp!

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v2

Here is a new patch that makes all the tests in bug 961325 pass, as far as script location information is concerned. JS::DescribeEntryPoint may be a little debugger-specific, but I assume it is OK if that is going to be the only use case for this API.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8425603 [details] [diff] [review]
v2

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

::: dom/base/ScriptSettings.h
@@ +178,5 @@
>  
>  private:
>    JSAutoCompartment mAc;
>    dom::ScriptSettingsStack& mStack;
> +  JS::AutoSaveEntryPoint mEntryPointOverride;

As noted before, this shouldn't be called "Override".

::: js/src/jsapi.cpp
@@ +6457,5 @@
> +{
> +    MOZ_ASSERT(cx);
> +
> +    // If there's no accessible activation on the stack, we'll return null from
> +    // DescribeEntryPoint anyway, so there's no need to annotate anything.

This isn't quite right. In the case of the bottom-most entry point (that presumably happens before we push any activations), we still want to return non-null from DescribeEntryPoint. So we should have an implicit entry point at the bottom of the JS stack.

@@ +6480,5 @@
> +    if (lineno)
> +        *lineno = 0;
> +
> +    NonBuiltinFrameIter i(cx, FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED,
> +                               cx->compartment()->principals);

This isn't right. You need to iterate until you hit an activation with a non-zero entryPoint, _or_ the bottommost activation (per the convention about an implicit entry point above).

@@ +6481,5 @@
> +        *lineno = 0;
> +
> +    NonBuiltinFrameIter i(cx, FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED,
> +                               cx->compartment()->principals);
> +    // Skip the first frame as it belongs to the debugger.

Hm, I don't think this is the right solution here. Why is the debugger pushing an entry point?

::: js/src/jsapi.h
@@ +4897,5 @@
> +DescribeEntryPoint(JSContext *cx, AutoFilename *filename = nullptr,
> +                   unsigned *lineno = nullptr);
> +
> +extern JS_PUBLIC_API(void)
> +SaveEntryPoint(JSContext *cx);

I think "PushEntryPoint" and "PopEntryPoint" would be the most appropriate naming here (and be sure to change the internal accessors on js::Activation as well).

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v3

So is this what you had in mind? The problem I'm having with this setup is that the tests from bug 961325 fail in various (but not all) cases to provide the right URL and line number. In most failures the topmost debugger frame has an entry point, especially when the test goes in and out of debugger frames more than once.

I'm not entirely sure that the failures are not caused by the way the tests are written, either.

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8426337 [details] [diff] [review]
v3

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

Thinking a bit more, I think the ideal naming is MarkEntryPoint and UnmarkEntryPoint - can you update all of the relevant names (the RAII class, the public API, and the internal accessors)? Sorry to change it up. :P

::: dom/base/ScriptSettings.h
@@ +178,5 @@
>  
>  private:
>    JSAutoCompartment mAc;
>    dom::ScriptSettingsStack& mStack;
> +  JS::AutoSaveEntryPoint mEntryPoint;

Name-wise, I think this should be:

JS::AutoMarkEntryPoint mJSEntryMarker;

::: js/src/jsapi.cpp
@@ +6452,5 @@
>      act->unhideScriptedCaller();
>  }
>  
> +JS_PUBLIC_API(void)
> +PushEntryPoint(JSContext *cx)

I think these should take a JSRuntime directly, since they'll generally be instantiated at a moment where we're still sorting out the proper cx.

@@ +6454,5 @@
>  
> +JS_PUBLIC_API(void)
> +PushEntryPoint(JSContext *cx)
> +{
> +    MOZ_ASSERT(cx);

No need to assert against conditions that will segfault anyway.

@@ +6478,5 @@
> +    bool found = false;
> +    if (lineno)
> +        *lineno = 0;
> +
> +    for (NonBuiltinFrameIter i(cx, FrameIter::ALL_CONTEXTS, FrameIter::GO_THROUGH_SAVED,

We only want scripted frames here, right? We should probably make this a NonBuiltinScriptFrameIter.

@@ +6486,5 @@
> +    {
> +        if (filename)
> +            filename->reset(i.scriptSource());
> +        if (lineno)
> +            *lineno = i.computeLine();

This works, but is inefficient, since we have to compute the line and filename at every frame and then throw it away. Am I correct that we won't be using this in hot code? If so, please add a comment indicating that the current strategy prioritizes simplicity over performance, and that a more performant (and more complicated) approach would be to add a hasNext() method to Frameiter.

@@ +6493,5 @@
> +        // XXX:debug
> +        printf(">>>>>>>>>>>>>>>>> %s:%d, ", i.scriptSource()->filename(), i.computeLine());
> +        printf("has %sentry point, ", i.activation()->hasSavedEntryPoint()? "": "no ");
> +        printf("is %slast activation\n", !i.activation()->prev()? "": "not ");
> +        // XXX:debug

This will need to go, of course. ;-)

@@ +6495,5 @@
> +        printf("has %sentry point, ", i.activation()->hasSavedEntryPoint()? "": "no ");
> +        printf("is %slast activation\n", !i.activation()->prev()? "": "not ");
> +        // XXX:debug
> +
> +        if (i.activation()->hasSavedEntryPoint() || !i.activation()->prev())

This second condition looks wrong - why is it necessary? Activations can have multiple frames in them, so this will currently give you the topmost frame on the bottommost activation, which isn't what you want.

This is probably the reason for the incorrect results you were seeing.

@@ +6500,5 @@
> +            break;
> +    }
> +
> +    if (!found)
> +        return false;

I would move the *FrameIter out of the loop header, and move the |return false| immediately after the declaration.

Note that this will return false if somebody does setTimeout(someNativeFunction, 0) and |someNativeFunction| ends up query the execution reason. Is this a case that we need to handle?

::: js/src/jsapi.h
@@ +4902,5 @@
> +
> +extern JS_PUBLIC_API(void)
> +PopEntryPoint(JSContext *cx);
> +
> +class AutoSaveEntryPoint

This is misnamed

::: js/src/vm/Stack.h
@@ +1155,5 @@
>      // data structures instead.
>      size_t hideScriptedCallerCount_;
>  
> +    // Counter incremented by JS::SaveEntryPoint and decremented by
> +    // JS::RestoreEntryPoint.

This comment needs updating.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

> This works, but is inefficient, since we have to compute the line and
> filename at every frame and then throw it away. Am I correct that we won't
> be using this in hot code? If so, please add a comment indicating that the
> current strategy prioritizes simplicity over performance, and that a more
> performant (and more complicated) approach would be to add a hasNext()
> method to Frameiter.

Yes, it is only going to be run when a debugger is inspecting the page and execution is paused.

> > +        if (i.activation()->hasSavedEntryPoint() || !i.activation()->prev())
> 
> This second condition looks wrong - why is it necessary? Activations can
> have multiple frames in them, so this will currently give you the topmost
> frame on the bottommost activation, which isn't what you want.
> 
> This is probably the reason for the incorrect results you were seeing.

This was my attempt to check for the bottommost activation, which would count as an implicit entry point. If I understand things correctly, activations form a stack with prev() pointing at the previous one. Is there a better way to check for that?

Comment 30

[Bobby Holley (:bholley)]

(In reply to Panos Astithas [:past] from comment #29)
> This was my attempt to check for the bottommost activation, which would
> count as an implicit entry point. If I understand things correctly,
> activations form a stack with prev() pointing at the previous one. Is there
> a better way to check for that?

Yes. But activations contain multiple frames, so finding the bottom-most activation does not (from the start) give you the bottom-most frame.

In order to handle the implicit entry point, you just need to check i.done() immediately after instantiating the iterator. If that's true, then there are no scripted frames on the stack, and you can return false. Otherwise, you are guaranteed to that there is at least one scripted frame.

However, guarding for hasEntryPoint() is actually more tricky than I realized yesterday, because a ScriptedFrameIter might miss such a point before settling upon its first frame. So I think you need to build support for this into the iterator itself, along the lines of STOP_AT_SAVED.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v4

I finally figured out the root of the problem with incorrect URL and line information, and I'm embarassed to admit that it was a missing initialization of Activation::entryPoint_ :-) Now all tests in bug 961325 pass.

(In reply to Bobby Holley (:bholley) from comment #28)
> > +PushEntryPoint(JSContext *cx)
> 
> I think these should take a JSRuntime directly, since they'll generally be
> instantiated at a moment where we're still sorting out the proper cx.

Does that include DescribeEntryPoint as well? I assume not, because it's used in a different time than the other two.

> Note that this will return false if somebody does
> setTimeout(someNativeFunction, 0) and |someNativeFunction| ends up query the
> execution reason. Is this a case that we need to handle?

I don't think so. We only expose execution reasons through the debugger API which is meant for JS only, so I don't think we would support that.

Comment 32

[Bobby Holley (:bholley)]

Comment on attachment 8429388 [details] [diff] [review]
v4

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

(In reply to Panos Astithas [:past] from comment #31)
> > I think these should take a JSRuntime directly, since they'll generally be
> > instantiated at a moment where we're still sorting out the proper cx.
> 
> Does that include DescribeEntryPoint as well? I assume not, because it's
> used in a different time than the other two.

Yeah, DescribeEntryPoint is fine. In general most APIs take JSContexts right now, and we'll convert them en masse when we get rid of JSContexts. I just don't want to force callers to do gymnastics to get a JSContext at an inconvenient time when we really just care about the JSRuntime.

> > Note that this will return false if somebody does
> > setTimeout(someNativeFunction, 0) and |someNativeFunction| ends up query the
> > execution reason. Is this a case that we need to handle?
> 
> I don't think so. We only expose execution reasons through the debugger API
> which is meant for JS only, so I don't think we would support that.

OK. That makes life a bit easier. ;-)

r=bholley with those comments addressed. Flagging luke for sr. Thanks for pushing this through!

::: dom/base/ScriptSettings.cpp
@@ +243,5 @@
>    : AutoJSAPI(aCx ? aCx : FindJSContext(aGlobalObject), aIsMainThread, /* aSkipNullAc = */ true)
>    , ScriptSettingsStackEntry(aGlobalObject, /* aCandidate = */ true)
>    , mAc(cx(), aGlobalObject->GetGlobalJSObject())
>    , mStack(ScriptSettingsStack::Ref())
> +  , mJSEntryMarker(CycleCollectedJSRuntime::Get()->Runtime())

It doesn't matter in practice, but I think it would be slightly more logical to initialize this before mAc (which involves reordering the members in the header file as well).

::: js/src/jsapi.cpp
@@ +6489,5 @@
> +        if (lineno)
> +            *lineno = i.computeLine();
> +
> +        ++i;
> +    } while (!i.done() && !i.activation()->hasEntryPoint());

Given that we have STOP_AT_ENTRY_POINT, we don't need the hasEntryPoint() check here, right? I'm pretty sure it should go away, unless I'm missing something.

::: js/src/jsapi.h
@@ +4890,5 @@
>      MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
>  };
>  
> +/*
> + * Return the current filename and line number of the latest entry point.

Add a comment indicating that entry points are created via MarkEntryPoint (which must be FILO-balanced with calls to UnmarkEntryPoint), and that we have an implicit entry point at the bottom of the JS call stack.

::: js/src/vm/Stack.cpp
@@ +636,5 @@
>  }
>  
>  FrameIter::Data::Data(ThreadSafeContext *cx, SavedOption savedOption,
> +                      ContextOption contextOption, JSPrincipals *principals,
> +                      EntryPointOption entryPointOption)

EntryPoint option should go immediately after SavedOption.

@@ +702,5 @@
> +}
> +
> +FrameIter::FrameIter(JSContext *cx, ContextOption contextOption,
> +                     SavedOption savedOption, JSPrincipals *principals,
> +                     EntryPointOption entryPointOption)

Here as well.

::: js/src/vm/Stack.h
@@ +1774,5 @@
> +    ScriptFrameIter(JSContext *cx,
> +                    ContextOption cxOption,
> +                    SavedOption savedOption,
> +                    JSPrincipals *prin,
> +                    EntryPointOption entryPointOption)

And here.

@@ +1835,5 @@
> +    NonBuiltinFrameIter(JSContext *cx,
> +                        FrameIter::ContextOption contextOption,
> +                        FrameIter::SavedOption savedOption,
> +                        JSPrincipals *principals,
> +                        FrameIter::EntryPointOption entryPointOption)

And here.

@@ +1886,5 @@
> +    NonBuiltinScriptFrameIter(JSContext *cx,
> +                              ScriptFrameIter::ContextOption contextOption,
> +                              ScriptFrameIter::SavedOption savedOption,
> +                              JSPrincipals *principals,
> +                              EntryPointOption entryPointOption)

And here.

Comment 33

[Luke Wagner [:luke]]

Comment on attachment 8429388 [details] [diff] [review]
v4

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

::: js/src/vm/Stack.cpp
@@ +545,5 @@
>              data_.state_ = DONE;
>              return;
>          }
>  
> +        // If JS_MarkEntryPoint was called, stop iterating here (unless

JS::MarkEntryPoint

::: js/src/vm/Stack.h
@@ +1591,5 @@
>          ThreadSafeContext * cx_;
>          SavedOption         savedOption_;
>          ContextOption       contextOption_;
>          JSPrincipals *      principals_;
> +        EntryPointOption    entryPointOption_;

Can you put this next to SavedOption?

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v5

Thanks for the prompt reviews, here is the updated patch. I'm going to push it to try along with the patch from bug 961325 and land them together after reviews, since the tests in that patch exercise these code paths as well.

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8430128 [details] [diff] [review]
v5

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

::: js/src/jsapi.cpp
@@ +6486,5 @@
> +        // approach would be to add a hasNext() method to Frameiter.
> +        if (filename)
> +            filename->reset(i.scriptSource());
> +        if (lineno)
> +            *lineno = i.computeLine();

Writing some more tests for bug 961325, I was wondering whether this bit is actually correct. If I understand FrameIter::computeLine correctly, it looks like this is the JS line that is about to be executed next, not necessarily the one that was originally executed when an execution reason was established. Is this accurate?

If it is, then this wouldn't describe the Debugger.Frame.prototype.executionReason accurately, according to the spec I've been working with.

Comment 36

[Bobby Holley (:bholley)]

Ah that's a good point. In that case, you just want i.script().lineno(). Does that give you the right thing?

Luke, do we need to worry about asmJS frames here?

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: v6

Thanks, that works as expected.

(In reply to Bobby Holley (:bholley) from comment #36)
> Luke, do we need to worry about asmJS frames here?

Do asmJS frames enter the picture even when I'm using NonBuiltinScriptFrameIter?

Comment 38

[Luke Wagner [:luke]]

That's right, NonBuiltinScriptFrameIter will skip over asm.js frames.  Normally, you'd use NonBuiltinFrameIter and stick to the methods that don't require hasScript():
  http://hg.mozilla.org/mozilla-central/file/56f9b7162d7c/js/src/vm/Stack.h#l1607
but currently there is not an analogue of i.script()->lineno.  (computeLine() returns the *current* line of the frame.)

For now, you can leave this as NonBuiltinScriptFrameIter and I'll fix it up later by adding some i.scriptFirstLine().

Comment 39

[Tom Tromey :tromey]

Attachment: Add a way to retrieve the file name and line number of the first bit of script that executes when the UA enters script (bug 1006291). r=bholley, sr=luke

Rebased onto today's fx-team.

Comment 40

[Panos Astithas (he/him) [:past] (please ni?)]

I never ended up landing this as I was waiting for the code that would be using it from bug 961325 to land as well. It doesn't sound like we will need to do this any more, but if that ever becomes the case, we can simply reopen the bug.