[webkit-reviews] review requested: [Bug 171537] WebAssembly: running out of executable memory should throw OoM : [Attachment 313960] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 171537: WebAssembly: running out of executable memory should throw OoM
https://bugs.webkit.org/show_bug.cgi?id=171537

Attachment 313960: patch

https://bugs.webkit.org/attachment.cgi?id=313960&action=review




--- Comment #10 from JF Bastien <jfbastien at apple.com> ---
Created attachment 313960

  --> https://bugs.webkit.org/attachment.cgi?id=313960&action=review

patch

(In reply to Mark Lam from comment #5)
> Comment on attachment 313772 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313772&action=review
> 
> > Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:299
> > -	 if (m_state == State::Compiled) {
> > +	 if (!failed() && m_state == State::Compiled) {
> 
> Why not just return early if already failed?	The only possible reason for
> not returning early is if you want to move the state to State::Complete. 
> However, this disagrees with the allocation failure cases below where you
> return early without setting the state to State::Complete.  So, which is
> behaving wrongly here?

Because I also want to run completion tasks, as done below, which can call back
into this function.


(In reply to Saam Barati from comment #6)
> Comment on attachment 313772 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313772&action=review
> 
> > Source/JavaScriptCore/wasm/js/JSWebAssemblyCodeBlock.cpp:63
> > +		 switch (binding.error()) {
> 
> Nit: I’m a fan of adding default: ASSERT_NOT_REACHED

That's a bad idea because right now it's a compile error if one of the cases
isn't covered. With default it because a runtime assert.