User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_4) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.54 Safari/536.5 Steps to reproduce: execute following script function test() { try { ''.match('('); } catch (e) { return true; } } Actual results: uncatchable out of memory error is raised Expected results: ''.match('(') raise SyntaxErorr, error is catched and returns true.

Thanks for providing a clear test case and a patch as well! I looked into this a bit more and I think the underlying problem isn't that there is a missing syntax check but, rather, that the syntax error is getting clobbered by the out of memory error: http://hg.mozilla.org/mozilla-central/file/b7dd74f5a7d2/js/src/vm/RegExpObject.cpp#l567

Another 'goto' bites the dust...

Comment on attachment 632031 [details] [diff] [review] fix and test Actually, this isn't regexp-specific, it's a general error and njn owes me a review or two ;)

Comment on attachment 632031 [details] [diff] [review] fix and test Review of attachment 632031 [details] [diff] [review]: ----------------------------------------------------------------- I do owe you a review or two. And I just learnt about mfbt/Scoped.h, nice. ::: js/public/Utility.h @@ +597,5 @@ > > +template <typename T> > +struct ScopedDeleteTraits > +{ > + typedef T *type; Is this typedef necessary? I see that ScopedFreePtr has it but ScopedDelete{Ptr,Array} do not... @@ +601,5 @@ > + typedef T *type; > + static T *empty() { return NULL; } > + static void release(T *ptr) { Foreground::delete_(ptr); } > +}; > +SCOPED_TEMPLATE(ScopedDelete, ScopedDeleteTraits) Adding a "Ptr" suffix would be more consistent with the names in mfbt/Scoped.h. But then we'd have two subtly different classes with the same name. "ScopedForegroundDeletePtr" is a mouthful but very clear, and would be best in this case, IMO. ::: js/src/vm/RegExpObject.cpp @@ +564,2 @@ > if (!shared) > + return false; This case used to call js_ReportOutOfMemory(), but now it doesn't. Is that intentional? @@ +575,5 @@ > > /* > * Since 'error' deletes 'shared', only guard 'shared' on success. This is > * safe since 'shared' cannot be deleted by GC until after the call to > * map_.add() directly above. Change |map_.add()| to |map_.relookupOrAdd()|, plz.

(In reply to Nicholas Nethercote [:njn] from comment #5) > Is this typedef necessary? I see that ScopedFreePtr has it but > ScopedDelete{Ptr,Array} do not... Obscuriously, ScopedDeletePtrTraits gets it by deriving ScopedFreePtrTraits. > Adding a "Ptr" suffix would be more consistent with the names in > mfbt/Scoped.h. But then we'd have two subtly different classes with the > same name. "ScopedForegroundDeletePtr" is a mouthful but very clear, and > would be best in this case, IMO. SM won't be able to use any of templates in Scoped.h (b/c we can't call delete/free directly), so it shouldn't be an actual conflict in practice. The "Foreground" distinction is kindof lame (I there was a bug to remove the background-free thread... maybe it has already happened?), so I'd really not like to embed "Foreground" in even more names. ScopedDeletePtr sounds good, though. > ::: js/src/vm/RegExpObject.cpp > @@ +564,2 @@ > > if (!shared) > > + return false; > > This case used to call js_ReportOutOfMemory(), but now it doesn't. Is that > intentional? Note the change from cx->runtime->new_ to cx->new_ (so js_ReportOutOfMemory is still called).

> Note the change from cx->runtime->new_ to cx->new_ (so js_ReportOutOfMemory > is still called). I did see that but I didn't realize the two functions had different semantics! How odd, and a little distasteful. Oh well.

(In reply to Nicholas Nethercote [:njn] from comment #7) Agreed. The current allocation naming scheme isn't quite right (for several reasons) and needs rejiggering.

Please re-nominate for tracking if this is believed to be a recent regression or has significant user impact.