Undefined value error in generated DOMProxyHandler::delete_

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: bzbarsky)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
DOMProxyHandler::delete_ looks like this in my tree:

> bool
> DOMProxyHandler::delete_(JSContext* cx, JS::Handle<JSObject*> proxy, JS::Handle<jsid> id, bool* bp) const
> {
>   MOZ_ASSERT(!xpc::WrapperFactory::IsXrayWrapper(proxy),
>             "Should not have a XrayWrapper here");
> 
>   int32_t index = GetArrayIndexFromId(cx, id);
>   if (IsArrayIndex(index)) {
>     bool result;
>     bool found = false;
>     mozilla::dom::TestIndexedDeleterWithRetvalInterface* self = UnwrapProxy(proxy);
>     self->IndexedDeleter(index, found);
>     MOZ_ASSERT(!JS_IsExceptionPending(cx));
>     if (found) {
>       *bp = result;
>     } else {
>       *bp = true;
>     }
>     // We always return here, even if the property was not found
>     return true;
>   }
> 
>   return dom::DOMProxyHandler::delete_(cx, proxy, id, bp);
> }

cppcheck says that |result| is used without being initialized. It is correct,
AFAICT.
(Reporter)

Comment 1

4 years ago
This is in $OBJDIR/dom/bindings/TestCodeGenBinding.cpp, BTW.
Yeah, ok.  So the good news is that there are no indexed deleters in any non-test code, and I hope there never will be.

Is it viable to just drop support for them instead of trying to fix this code to be sane?
Flags: needinfo?(peterv)
Flags: needinfo?(cam)
Yes I think we should drop support for indexed deleters.  I am reminded by looking at https://www.w3.org/Bugs/Public/show_bug.cgi?id=20535 that we do have a use of indexed creators. :(  But deleters can go as far as I know.
Flags: needinfo?(cam)
If not, then the point is that we should be assigning the return value of IndexedDeleter to "result" in this case.

The TestNamedDeleterWithRetvalInterface case has a similar issue...  Again, luckily our extant named deleters all return void.
CGCallGenerator is only assigning stuff to resultVar if needResultDecl.  That's wrong.
Created attachment 8551623 [details] [diff] [review]
Make sure to assign to resultVar in the binding call generator even if resultVar is predeclared, as long as we have a result to assign
Attachment #8551623 - Flags: review?(peterv)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8551623 - Flags: review?(peterv) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e51e022084
Flags: needinfo?(peterv)
Target Milestone: --- → mozilla38
https://hg.mozilla.org/mozilla-central/rev/f6e51e022084
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.