1083008 - nsPresShell.cpp:6940:9: warning: variable 'frameElement' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

New build warning (detected with clang):
{
$SRCDIR/layout/base/nsPresShell.cpp:6940:9: warning: variable 'frameElement' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    if (window) {
        ^~~~~~
$SRCDIR/layout/base/nsPresShell.cpp:6946:10: note: uninitialized use occurs here
  while (frameElement) {
         ^~~~~~~~~~~~
$SRCDIR/layout/base/nsPresShell.cpp:6940:5: note: remove the 'if' if its condition is always true
    if (window) {
    ^~~~~~~~~~~~
$SRCDIR/layout/base/nsPresShell.cpp:6927:24: note: initialize the variable 'frameElement' to silence this warning
  Element* frameElement;
}


This is from code just added in bug 989198:
http://hg.mozilla.org/mozilla-central/rev/f5a538109e36#l10.144

Code snippet:
> 6920 static void
> 6921 BuildTargetChainForBeforeAfterKeyboardEvent(nsINode* aTarget,
> 6922                                             nsTArray<nsCOMPtr<Element> >& aChain,
> 6923                                             bool& aTargetIsIframe)
> 6924 {
> 6925   nsCOMPtr<nsIContent> content(do_QueryInterface(aTarget));
> 6926   nsCOMPtr<nsPIDOMWindow> window;
> 6927   Element* frameElement;
> 6928 
> 6929   // Initialize frameElement.
> 6930   if (content && content->IsHTML(nsGkAtoms::iframe)) {
> 6931     aTargetIsIframe = true;
> 6932     frameElement = aTarget->AsElement();
> 6933   } else {
> 6934     // If event target is not an iframe, dispatch keydown/keyup event to its
> 6935     // window after dispatching before events to its ancestors.
> 6936     aTargetIsIframe = false;
> 6937 
> 6938     // And skip the event target and get its parent frame.
> 6939     window = aTarget->OwnerDoc()->GetWindow();
> 6940     if (window) {
> 6941       frameElement = window->GetFrameElementInternal();
> 6942     }
> 6943   }
> 6944 
> 6945   // Check permission for all ancestors and add them into the target chain.
> 6946   while (frameElement) {
> 6947     if (CheckPermissionForBeforeAfterKeyboardEvent(frameElement)) {
> 6948       aChain.AppendElement(frameElement);
> 6949     }
> 6950     window = frameElement->OwnerDoc()->GetWindow();
> 6951     frameElement = window ? window->GetFrameElementInternal() : nullptr;
> 6952   }

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#6920

As shown here, 'frameElement' is only sometimes initialized. In particular, if we fail the first "if" check & 'window' is null, then frameElement is uninitialized, and then we use it in a loop condition & dereference it in a loop body.

Gina, can you take a look and fix this in whatever way is appropriate?

Comment 1

[Gina Yeh [:gyeh] [:ginayeh]]

Sure! Thanks for filing this bug.

Comment 2

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Fix warning.

Comment 3

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 8505941 [details] [diff] [review]
Patch 1(v1): Fix warning.

Review of attachment 8505941 [details] [diff] [review]:
-----------------------------------------------------------------

Smaug, the patch is quite short and feel free to let me know if you have any concerns. Thanks.

Comment 4

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

Comment on attachment 8505941 [details] [diff] [review]
Patch 1(v1): Fix warning.

Odd to first assign value (something else than null) to frameElement and then
immediately assign some other value.
Why not

Element* frameElement;
if (aTargetIsIframe) {
  frameElement = aTarget->AsElement(); 
} else {
  nsPIDOMWindow* window = aTarget->OwnerDoc()->GetWindow();
  frameElement = window ? window->GetFrameElementInternal() : nullptr;
}

with that, r+

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Bug 989198 was backed out. Please remember to include this fix when it re-lands.