Closed Bug 681092 Opened 10 years ago Closed 7 years ago

Add script size assertions

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(2 files)

Attached patch patchSplinter Review
This patch tries to track down the issues identified in bug 670702 comment 18.
Attachment #554976 - Flags: review?(dmandelin)
Comment on attachment 554976 [details] [diff] [review]
patch

Never mind with the review. This is tripping on tryserver. Either I messed up or else the problem shouldn't be too hard to fix.
Attachment #554976 - Flags: review?(dmandelin)
Attached patch sourcenote patchSplinter Review
This fixes a small bug that caused overallocation for source notes. I also tried to simplify the code. Hopefully I didn't break anything in the process.

This is not the bug that I was looking for, but we need to fix it to make the assertions useful.
Attachment #555004 - Flags: review?(brendan)
Attachment #554976 - Flags: review?(dmandelin)
Did you mean to cancel the review, Dave? I'd still like to get the assertions in. I'm nearly certain that the srcnote patch won't fix the problem I'm tracking.
Attachment #554976 - Flags: review+
Comment on attachment 555004 [details] [diff] [review]
sourcenote patch

>+    /* Don't forget to account for the terminator. */
>+    *count = cg->prolog.noteCount + cg->main.noteCount + 1;
>+
>+    return JS_TRUE;

Feel free in this patch to s/JS_TRUE/true/g and s/JS_FALSE/false/g -- doing so makes it easier to abut the two non-empty lines cited above, which is more like house style.

Thanks for doing this needed cleanup. We talked too long ago and I failed to check my review q -- sorry! Please mail me when I do that.

/be
Attachment #555004 - Flags: review?(brendan) → review+
Bill: did you forget to land this r+'d patch from 2011? :) Are these script size assertions still relevant?
Flags: needinfo?(wmccloskey)
I think my intention here was to track down a crash that eventually was fixed in some other way. The code looks pretty different at this point, so it would probably be more trouble than it's worth to rebase these patches.
Flags: needinfo?(wmccloskey)
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.