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

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dholbert, Assigned: gyeh)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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?
(Reporter)

Updated

4 years ago
Flags: needinfo?(gyeh)
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 1

4 years ago
Sure! Thanks for filing this bug.
Assignee: nobody → gyeh
Flags: needinfo?(gyeh)
(Assignee)

Comment 2

4 years ago
Created attachment 8505941 [details] [diff] [review]
Patch 1(v1): Fix warning.
(Assignee)

Updated

4 years ago
Attachment #8505941 - Attachment description: Patch 1(v1): Fix warning: variable. → Patch 1(v1): Fix warning.
(Assignee)

Comment 3

4 years ago
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.
Attachment #8505941 - Flags: review?(bugs)
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+
Attachment #8505941 - Flags: review?(bugs) → review+
Bug 989198 was backed out. Please remember to include this fix when it re-lands.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.