Closed
Bug 599481
Opened 14 years ago
Closed 14 years ago
static string tables should be constant
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
Details
(Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib])
Attachments
(1 file, 2 obsolete files)
9.26 KB,
patch
|
alangpierce
:
review+
|
Details | Diff | Splinter Review |
JSString::flatClearMutable is implemented using: inline void flatClearMutable() { JS_ASSERT(isFlat()); mLengthAndFlags &= ~MUTABLE; } This will write to the mLengthAndFlags field even when MUTABLE is not set. This may result in a crash when the method is called on a static string like int or unit strings that may come from write-protected memory (like happens on Windows). I have discovered this during testing for bug 596229 with crash coming from JS_GetStringBytes calling flatClearMutable unconditionally.
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 1•14 years ago
|
||
Attachment #478390 -
Flags: review?(alangpierce)
Assignee | ||
Comment 2•14 years ago
|
||
The patch extends the testIntString with more coverage. To bad that was not helpful in catching this bug as we also need I suppose special compilation flags to ensure that the static strings that loader has to relocate are in the read-only memory.
Attachment #478390 -
Attachment is obsolete: true
Attachment #478396 -
Flags: review?(alangpierce)
Attachment #478390 -
Flags: review?(alangpierce)
Assignee | ||
Comment 3•14 years ago
|
||
The bug is bogus as presently static string tables do not have const keyword so the compiler must not use read-only memory for them. Still it would be nice to make the strings const to make sure that any mutation of their content is caught as early as possible if the platform can write-protect their memory. I morph the bug accordingly.
No longer blocks: 571549
Summary: JSString::flatClearMutable must write to mLengthAndFlags only for mutable strings → static string tables should be constant
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #478396 -
Attachment is obsolete: true
Attachment #478427 -
Flags: review?(alangpierce)
Attachment #478396 -
Flags: review?(alangpierce)
Comment 5•14 years ago
|
||
Comment on attachment 478427 [details] [diff] [review] const tables Apparently, I no longer have the bits to do official reviews, even though I'm flagged for review, so I'll just do the review in a comment... You should assert !isStatic() at the start of the following JSString methods: initFlat initFlatMutable flatSetAtomized initDependent initTopNode since callers now need to be careful that they aren't doing anything that writes to a static string (even if the string doesn't change value). With those added, r+.
Comment 6•14 years ago
|
||
(In reply to comment #5) > > Apparently, I no longer have the bits to do official reviews fixed?
Updated•14 years ago
|
Attachment #478427 -
Flags: review?(alangpierce) → review+
Comment 7•14 years ago
|
||
Yep, fixed. Thanks.
Assignee | ||
Comment 8•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/71fa2c4f4cf8 - pushed with extra asserts
Whiteboard: fixed-in-tracemonkey
Comment 9•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/71fa2c4f4cf8
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med]
Comment 10•14 years ago
|
||
A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :( Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386 New : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5 Change : +10.475 (2.49% / z=2.847) Graph : http://mzl.la/bZFaB3 The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med] → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css]
Comment 11•14 years ago
|
||
A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :( Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386 New : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5 Change : -112.809 (-5.6% / z=2.787) Graph : http://mzl.la/9gFu4n The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css] → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib]
Comment 12•14 years ago
|
||
A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :( Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386 New : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5 Change : -11.226 (-8.8% / z=2.659) Graph : http://mzl.la/bZu5UP The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Comment 13•14 years ago
|
||
A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :( Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386 New : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5 Change : -109.155 (-5.34% / z=2.197) Graph : http://mzl.la/b0dlou The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Comment 14•14 years ago
|
||
A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :( Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386 New : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5 Change : -11.749 (-9.06% / z=2.866) Graph : http://mzl.la/avZij4 The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
You need to log in
before you can comment on or make changes to this bug.
Description
•