Closed Bug 868448 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: jib, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
Thank you for catching this!
Assignee: nobody → bzbarsky
Summary: Webidl codegen bug: optional dictionary arg=null asserts on null → Webidl codegen bug: optional dictionary arg=null asserts on null when converting to JS
Whiteboard: [need review]
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?
Attachment #745192 - Flags: review?(bugs) → review+
> 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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/47b7673201e5
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla23
https://hg.mozilla.org/mozilla-central/rev/47b7673201e5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.