897919 - Handle lazy scripts in debugger.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

I stumbled upon this while debugging another issue. I can reproduce a crash caused by the `nonLazyScript` in `DebuggerObject_getScript`, but the STR are kinda annoying. I can provide them, if you want me to, of course, but this is a pretty straight-forward change.

The other changed locations look equally fishy to me, but maybe with some knowledge about how exactly they're used, one can be sure that they never deal with lazy scripts. In that case, I'd happily revert them, of course.

Comment 1

[Till Schneidereit [:till]]

Attachment: Handle lazy scripts in debugger

Comment 2

[Jim Blandy :jimb]

Comment on attachment 780902 [details] [diff] [review]
Handle lazy scripts in debugger

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

I think this definitely needs regression tests. Is it possible to capture the steps to reproduce in a js/src/jit-test/tests/debug test?

Comment 3

[Jim Blandy :jimb]

For the record, I'm quite confident the fix is correct. But I'm mystified as to why this wasn't caught by the unit tests. There must be some systemic reason our tests never hit lazy functions, which is quite worrisome. I'd very much like to understand that part of the story before we proceed; we might be able to catch many other bugs.

For what it's worth: the patch covers both DebuggerObject_getScript and DebuggerFrame_getScript. In the latter case, the use of nonLazyScript might be justified: we can't have stack frames running lazy scripts, can we?

Comment 4

[Jim Blandy :jimb]

Attachment: Correctly de-lazify functions before trying to inspect their scripts.

This patch has regression tests, deals with cross-compartment issues, and assumes that once a stack frame is running a function, it's no longer lazy.

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 784102 [details] [diff] [review]
Correctly de-lazify functions before trying to inspect their scripts.

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

Seems reasonable, based only on staring at the patch.

Comment 6

[Jim Blandy :jimb]

https://tbpl.mozilla.org/?tree=Try&rev=96fba6ad4ad7

Comment 7

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b27b567e0bdc

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b27b567e0bdc

Comment 10

[bhavana bajaj [:bajaj]]

Should we uplift this to Firefox 25 to resolve Bug 898867  ? Not sure if this already made it.

Comment 11

[Jim Blandy :jimb]

Yes, this patch is in Firefox 25; that's why I set the "target milestone" to "mozilla25". Isn't that our process for answering these questions?

I checked the mozilla-aurora head to be sure, and the patch's changes are present there.