Closed Bug 602003 Opened 14 years ago Closed 14 years ago

JM: Assert on Firebug test for issue 2914 when system URL filter is disabled

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dmandelin, Assigned: dmandelin)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

Spun off from bug 601197 comment 3:

> STR:
> 1) Install Firebug 1.6b1
> 1.5) set the following preference to false.
>
> user_pref("extensions.firebug.service.filterSystemURLs", false);
>
> 2) Open: http://getfirebug.com/tests/content/console/2914/issue2914.html
> 3) CRASH

This asserts on a 1-byte PC difference in jsd_scpt.c. It is related to the recent addition of the JSOP_BEGIN opcode. It doesn't make sense to trap on JSOP_BEGIN, so traps set there go to the next op. Evidently something about that is wrong.
Attached patch Patch (obsolete) — Splinter Review
This patch stops the crash, but I'm not sure it is the right thing to do. Odvarko, I'm going to need some advice from you on this one. Here's what's going on:

The Problem: The problem is that some JSScripts (but not all) now begin with a JSOP_BEGIN bytecode, and it is not valid to set a trap on that bytecode. Currently, something called from the new script hook tries to set a trap on bytecode offset 0 from script start (via jsdScript::SetBreakpoint / ClearBreakpoint).

Solution in the patch: What this patch does is to modify SetBreakpoint / Clearbreakpoint so that if the argument they are given corresponds to a JSOP_BEGIN bytecode, it instead sets the trap on the next bytecode. This stops the assert/immediate failure. (I also changed the JSDBGAPI SetTrap function so that it will generate an error if something tries to set a trap on a JSOP_BEGIN, so that in any case it will not crash.) 

The possible problem here is that the trap PC will not correspond to the original PC argument passed--this might confuse things later on in Firebug, but I wouldn't know about that.

Possibly better solution: I could add a new API to jsdScript of SetScriptStartBreakpoint / ClearScriptStartBreakpoint (naming suggestions welcome). It would set the breakpoint at the first valid trap breakpoint in the script. I could also make them return the PC actually used, or add a 3rd new API of GetScriptStartBreakpoint or some such, if Firebug needs that info.

The key questions here are, what exactly is Firebug trying to do in this hook, and what does it need to happen? Especially, how much does Firebug need to know about exactly what bytecode got the breakpoint?
Attachment #481048 - Flags: feedback?(odvarko)
(In reply to comment #1)
> Created attachment 481048 [details] [diff] [review]
> Patch
> 
> This patch stops the crash, but I'm not sure it is the right thing to do.
> Odvarko, I'm going to need some advice from you on this one. Here's what's
> going on:

Up to now the JS debugger has been my code (something we are working to change in Firebug 1.7)

> 
> The Problem: The problem is that some JSScripts (but not all) now begin with a
> JSOP_BEGIN bytecode, and it is not valid to set a trap on that bytecode.
> Currently, something called from the new script hook tries to set a trap on
> bytecode offset 0 from script start (via jsdScript::SetBreakpoint /
> ClearBreakpoint).

This is our mechanism for obtaining the call site of compilation so we we can correct the line numbers for eval().  See our WWW2010 paper,
http://getfirebug.com/doc/breakpoints/paper/breakpoints.pdf 
see also
Bug 332176

> 
> Solution in the patch: What this patch does is to modify SetBreakpoint /
> Clearbreakpoint so that if the argument they are given corresponds to a
> JSOP_BEGIN bytecode, it instead sets the trap on the next bytecode. This stops
> the assert/immediate failure. (I also changed the JSDBGAPI SetTrap function so
> that it will generate an error if something tries to set a trap on a
> JSOP_BEGIN, so that in any case it will not crash.) 
> 
> The possible problem here is that the trap PC will not correspond to the
> original PC argument passed--this might confuse things later on in Firebug, but
> I wouldn't know about that.

Yes that will be a problem: we have to clear the breakpoint and our only tool for this takes a PC. We can't test if a breakpoint is set. So we just clear PC = 0.

> 
> Possibly better solution: I could add a new API to jsdScript of
> SetScriptStartBreakpoint / ClearScriptStartBreakpoint (naming suggestions
> welcome). It would set the breakpoint at the first valid trap breakpoint in the
> script. I could also make them return the PC actually used, or add a 3rd new
> API of GetScriptStartBreakpoint or some such, if Firebug needs that info.

Any of these are fine, but one complication to keep in mind; the Firebug user may also set a breakpoint.

> 
> The key questions here are, what exactly is Firebug trying to do in this hook,
> and what does it need to happen? Especially, how much does Firebug need to know
> about exactly what bytecode got the breakpoint?

Hopefully I explained enough above. I have to wonder how much sense it makes for you to add hacky API to make my hacky code work, when what should have been done is well understood as described in Bug 449464.  But at this point in time I can't see how we have much choice. We have plans for a major re-write of all this stuff so I guess we can justify some hackiness and hope good things happen soon.
Attached patch v2 (obsolete) — Splinter Review
Review requested for the approach and API--I'll get other reviewers for the engine parts. This fixes the immediate bug by returning an error if the user tries to trap JSOP_BEGIN. The new API normalizePCForBreakpoint gives a PC for a breakpoint that will actually work--this can be used for the break-on-start functionality and also general breakpoints.
Assignee: general → dmandelin
Attachment #481048 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #481105 - Flags: review?(johnjbarton)
Attachment #481048 - Flags: feedback?(odvarko)
Based on the IDL comment I suggest
normalizePcForBreakpoint -> nearestPcForBreakpoint or nearestBreakablePC

For PC=0 this API seems ok, since we can only go in one direction.

For regular breakpoints 'nearest' could be an issue. Currently we use jsdIScript.lineToPc() then setBreakpoint on the result. If nearestPcForBreakpoint results in a PC such that pcToLine gives a different line, I guess users are unhappy and will report bugs.  "I said line 5 dammit!" Can we prevent that?

PC goes into setBreakpoint, clearBreakpoint and pcToLine. The IDL comment says we need to call the new function for setBreakpoint and clearBreakpoint. Do we need to call it for pcToLine?

Generally we will use the input to setBreakpoint as the later input to clearBreakpoint. If we've moved the PC before setBreakpoint we should not need to call before clearBreakpoint. 

It would be special if passing a PC out of range does not crash, since we can't known the max PC.  If I put in a large PC (int max), will I get the max PC value? Seems like the comment in the IDL means yes.

(That means I can now loop over PC = 0, PCmax and call pcToLine to give the executable lines (not currently feasible because we don't know PCmax). I don't see any issue this would cause). 

This will us to write specific code for FF4.0, we need to ask Boris if we can test for the existence of this new function.
(In reply to comment #4)
> 
> This will us to write specific code for FF4.0, 

That will be required.
(In reply to comment #5)
> (In reply to comment #4)
> > 
> > This will us to write specific code for FF4.0, 
> 
> That will be required.

Sure, but the question remains: can we test for IDL function rather than API version which can be difficult to make reliable.
(In reply to comment #4)
> Based on the IDL comment I suggest
> normalizePcForBreakpoint -> nearestPcForBreakpoint or nearestBreakablePC

Sure.

> For PC=0 this API seems ok, since we can only go in one direction.
> 
> For regular breakpoints 'nearest' could be an issue. Currently we use
> jsdIScript.lineToPc() then setBreakpoint on the result. If
> nearestPcForBreakpoint results in a PC such that pcToLine gives a different
> line, I guess users are unhappy and will report bugs.  "I said line 5 dammit!"
> Can we prevent that?

More background: There is really only one case where nearestBreakablePC will return a value other than its input: when the PC is the first bytecode of a script. In that case, the return value may be input+1, when the first bytecode is the special setup JSOP_BEGIN bytecode. That's it. And, in that case, the return value is definitely the first "real" bytecode of the function. (The reason we can't break there is that JSOP_BEGIN is actually part of the function call setup--it would be like breaking at a funny point in the middle of doing a function call.)

So, it will never change the line of code.

> PC goes into setBreakpoint, clearBreakpoint and pcToLine. The IDL comment says
> we need to call the new function for setBreakpoint and clearBreakpoint. Do we
> need to call it for pcToLine?

No.

> Generally we will use the input to setBreakpoint as the later input to
> clearBreakpoint. If we've moved the PC before setBreakpoint we should not need
> to call before clearBreakpoint. 

That is correct. I had included a sentence about that in the comment, but I omitted it as I thought it would be apparent. It was for you; but is there potential confusion so I should add back in the clarification?

> It would be special if passing a PC out of range does not crash, since we can't
> known the max PC.  If I put in a large PC (int max), will I get the max PC
> value? Seems like the comment in the IDL means yes.
> 
> (That means I can now loop over PC = 0, PCmax and call pcToLine to give the
> executable lines (not currently feasible because we don't know PCmax). I don't
> see any issue this would cause). 

I'm not sure I completely understand the above, but I think we are OK. It seems that you are iterating PCs with pcToLine, and the behavior there will be unchanged.

Passing PCs that are not in a script to nearestBreakablePC is invalid and may crash, but given that you don't need to call that before calling pcToLine, I think we are OK. Is that right?

> This will us to write specific code for FF4.0, we need to ask Boris if we can
> test for the existence of this new function.

OK.

I'll post a new patch with the new name, so we stay up to date, but otherwise I'll wait until you get back to me so we can make sure this will actually enable Firebug to work.
Attached patch v3 (rename new method) (obsolete) — Splinter Review
Attachment #481105 - Attachment is obsolete: true
Attachment #481288 - Flags: review?(johnjbarton)
Attachment #481105 - Flags: review?(johnjbarton)
Thinking about this a little more, it seems like maybe instead of this API, a new API for "get me the first 'real' PC in the script" might be better? It could be used in the break-on-script-entry case easily. For user-set breakpoints, I guess the Firebug code would have to check whether it's in range, and if not, increment by 1.

This seems cleaner on an API design level, but maybe it requires a bit more modification to Firebug to use?
(In reply to comment #9)
> Thinking about this a little more, it seems like maybe instead of this API, a
> new API for "get me the first 'real' PC in the script" might be better? It

Yes, that narrows the meaning, which is good here.

> could be used in the break-on-script-entry case easily. For user-set
> breakpoints, I guess the Firebug code would have to check whether it's in
> range, and if not, increment by 1.

? I would:
   var firstValidPC = script.getFirstValidPC();
   pc = (pc < firstValidPC) ? firstValidPC : pc;


> 
> This seems cleaner on an API design level, but maybe it requires a bit more
> modification to Firebug to use?

No problem; this solution reduces questions.
Another API design improvement could be having script.getLastValidPC() as a complementary function to the getFirstValidPC(), this way Firebug could easily iterate the range to get executable lines, no?

Honza
This one provides the new APIs suggested in comments 10 and 11. I used "EndPC" instead of "LastPC" to provide a Python-style start/end range.
Attachment #481288 - Attachment is obsolete: true
Attachment #481643 - Flags: review?(johnjbarton)
Attachment #481288 - Flags: review?(johnjbarton)
The function names are clear but the comment in the IDL is puzzling:
+     * Return the address just after the last bytecode in the script.
I would hope for something like:
+     * Return the last valid value of the PC (just after the last bytecode in the script)

The caller does not want an address and does not know about bytecodes really, so the API can only be about PC values.

Anyway this API will work for us and won't be a big change. Thanks!
Attachment #481643 - Attachment is patch: true
Attachment #481643 - Attachment mime type: application/octet-stream → text/plain
Attached patch v5 (doc improvement) (obsolete) — Splinter Review
Attachment #481643 - Attachment is obsolete: true
Attachment #481704 - Flags: review?(sayrer)
Attachment #481643 - Flags: review?(johnjbarton)
Attachment #481643 - Flags: review+
Attachment #481704 - Flags: review?(sayrer) → review+
http://hg.mozilla.org/tracemonkey/rev/af020f2b9293
Whiteboard: fixed-in-tracemonkey
Attached patch v6Splinter Review
- Fixes x64 build bustage

- x64 build bustage reminded me that the jsdI API wants to talk about offsets from script->code, not actual PCs, so we do that now.

- Added Mochitest for the new APIs.
Attachment #481704 - Attachment is obsolete: true
Attachment #481725 - Flags: review?(sayrer)
Forgot to mention I backed out the original push due to x64 build bustage: 

  http://hg.mozilla.org/tracemonkey/rev/a9ec8c6319fb
Whiteboard: fixed-in-tracemonkey
Attachment #481725 - Attachment is patch: true
Attachment #481725 - Attachment mime type: application/octet-stream → text/plain
Attachment #481725 - Flags: review?(sayrer) → review+
Relanded with fixes:

http://hg.mozilla.org/tracemonkey/rev/1998fa240434
Whiteboard: fixed-in-tracemonkey
As a reminder, I do appreciate and expect people to bump the iid of jsdI interfaces. You're welcome to do whatever you like in your playground, but please respect mine. Also adding methods in the middle of an interface table is absolutely unwelcome.

    1.11 +    unsigned long getFirstValidPC ();
    1.16 +    unsigned long getEndValidPC ();

I'm not fond of 'valid'. What's wrong with 'firstPC' and 'lastPC'?

Note that there's really absolutely no reason for these to be methods instead of attributes.

Please do ask me for review in the future. r-.
We had already planned to back out this change: we are removing JSOP_BEGIN, and this change is only required to make the debuggers work with JSOP_BEGIN. So it will be going away soon.
http://hg.mozilla.org/mozilla-central/rev/af020f2b9293
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
I don't understand what happened here: comment 20 says "We had already planned to back out this change", comment 21 shows it being pushed onto mozilla-central. 

Last nights build seems to fail setBreakpoint(0), which is why I ask...
See Bug 608763 for more about the setBreakpoint(0) problem.
Honza
You need to log in before you can comment on or make changes to this bug.