868448 - Webidl codegen bug: optional dictionary arg=null asserts on null when converting to JS

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Info.txt (callstack and relevant code snippets)

See info.txt for callstack, webidl and faulty generated code.

[11:20am] bz: This is totally a dictionary codegen bug
[11:21am] bz: I mean, it checks for null
[11:21am] bz: and sets it to null
[11:21am] bz: and then tries to wrap as an object

Comment 1

[Boris Zbarsky [:bzbarsky]]

Thank you for catching this!

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the successCode in dictionary to-js conversions to actually work right, and document the requirements on successCode better.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 745192 [details] [diff] [review]
Fix the successCode in dictionary to-js conversions to actually work right, and document the requirements on successCode better.

s/succesCode/successCode/

Do we really need do-while for scoping? Wouldn't {} be enough?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Do we really need do-while for scoping? 

Yes, that's what fixes the bug.  I need it not for the scoping but so the 'break' will do something useful.  Basically it converts this old codegen:


  {
    // scope for 'temp' and 'currentValue'
    JS::Value temp;
    const nsRefPtr<mozilla::dom::TestInterface>& currentValue = mSomeNullableInterface;
    if (!currentValue) {
      temp = JSVAL_NULL;
      if (!JS_DefinePropertyById(cx, obj, someNullableInterface_id, temp, nullptr, nullptr, JSPROP_ENUMERATE)) {
        return false;
      }
    }
    if (!WrapNewBindingObject(cx, parentObject, currentValue, &temp)) {
      MOZ_ASSERT(JS_IsExceptionPending(cx));
      return false;
    }
    if (!JS_DefinePropertyById(cx, obj, someNullableInterface_id, temp, nullptr, nullptr, JSPROP_ENUMERATE)) {
      return false;
    }
  }

into this:

  do {
    // block for our 'break' successCode and scope for 'temp' and 'currentValue'
    JS::Rooted<JS::Value> temp(cx);
    const nsRefPtr<mozilla::dom::TestInterface>& currentValue = mSomeNullableInterface;
    if (!currentValue) {
      temp = JSVAL_NULL;
      if (!JS_DefinePropertyById(cx, obj, someNullableInterface_id, temp, nullptr, nullptr, JSPROP_ENUMERATE)) {
        return false;
      }
      break;
    }
    if (!WrapNewBindingObject(cx, parentObject, currentValue, temp.address())) {
      MOZ_ASSERT(JS_IsExceptionPending(cx));
      return false;
    }
    if (!JS_DefinePropertyById(cx, obj, someNullableInterface_id, temp, nullptr, nullptr, JSPROP_ENUMERATE)) {
      return false;
    }
    break;
  } while(0);

which has quite different behavior if !currentValue.

Comment 5

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/47b7673201e5

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47b7673201e5