1300769 - [Static Analysis][Dereference after null check] In function nsXBLPrototypeHandler::ReportKeyConflict

Status: RESOLVED FIXED

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

Description

[Andi [:andi]]

The Static Analysis tool Coverity detected that |aKeyElement| is dereferenced after it's null checked but in a different context.

Null check:

>>  } else if (aKeyElement) {
>>    doc = aKeyElement->OwnerDoc();
>>  }

Dereference:

>>  aKeyElement->GetAttr(kNameSpaceID_None, nsGkAtoms::id, id);

I think the correct approach here is to call ReportKeyConflict only if in the callee function (mType & NS_HANDLER_TYPE_XUL) == true

Comment 1

[Andi [:andi]]

Attachment: Bug 1300769 - call ConstructPrototype only if mType is NS_HANDLER_TYPE_XUL.

Review commit: https://reviewboard.mozilla.org/r/76950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76950/

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8788449 [details]
Bug 1300769 - call ConstructPrototype only if mType is NS_HANDLER_TYPE_XUL.

https://reviewboard.mozilla.org/r/76950/#review75072

I cannot review XBL code. Ask a XBL peer.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8788449 [details]
Bug 1300769 - call ConstructPrototype only if mType is NS_HANDLER_TYPE_XUL.

https://reviewboard.mozilla.org/r/76950/#review75176

r=me with an additional assertion to help clarify the control flow.

::: dom/xbl/nsXBLPrototypeHandler.cpp:739
(Diff revision 1)
>                                            const char16_t* aAllowUntrusted)
>  {
>    mType = 0;
>  
>    if (aKeyElement) {
>      mType |= NS_HANDLER_TYPE_XUL;

Please add MOZ_ASSERT(!mPrototypeBinding) to make the relationship between `mType & NS_HANDLER_TYPE_XUL` and `aKeyElement` more clear.

Comment 4

[Andi [:andi]]

Comment on attachment 8788449 [details]
Bug 1300769 - call ConstructPrototype only if mType is NS_HANDLER_TYPE_XUL.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76950/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6cb70f31562
call ConstructPrototype only if mType is NS_HANDLER_TYPE_XUL. r=mrbkap

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6cb70f31562