Closed Bug 1216945 Opened 4 years ago Closed 4 years ago

Uninitialised value use in nsXULWindow::SizeShellTo

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: jseward, Unassigned)

Details

Attachments

(2 files)

STR: take a --enable-valgrind --disable-jemalloc build, and run

./mach mochitest --valgrind=/usr/bin/valgrind \
    js/xpconnect/tests/chrome/test_bug1124898.html

Analysis of the problem:

xfpe/appshell/nsXULWindow.cpp:1028 nsXULWindow::OnChromeLoaded() has this

          int32_t width, height;
          cv->GetContentSize(&width, &height);
          treeOwner->SizeShellTo(docShellAsItem, width, height);

The call ->GetContentSize is to layout/base/nsDocumentViewer.cpp:3350,
nsDocumentViewer::GetContentSize

This returns NS_ERROR_FAILURE, without writing to *aWidth or *aSize, here:

  nsIFrame *root = presShell->GetRootFrame();
  NS_ENSURE_TRUE(root, NS_ERROR_FAILURE);

but OnChromeLoaded() ignores the failure and passes the still-uninitialised
width/height values to SizeShellTo().  This tests them (indirectly) here:

  if (widthDelta || heightDelta) {

and at that point V complains.
Attached patch A possible fixSplinter Review
Attachment #8676769 - Flags: review?(nfroyd)
Comment on attachment 8676769 [details] [diff] [review]
A possible fix

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

This seems reasonable, though I'm not really sure that SizeShellTo would be ready to deal with width=height=0.  I see that nsGlobalWindow just gives up if GetContentSize fails.  A better solution would be:

  if (NS_SUCCEEDED(cv->GetContentSize(...))) {
    treeOwner->SizeShellTo(...);
  }

I guess continuing to initialize width and height in that scenario would be desirable, too?  Forwarding review to bz, who almost certainly knows more about this than I do.
Attachment #8676769 - Flags: review?(nfroyd)
Attachment #8676769 - Flags: review?(bzbarsky)
Attachment #8676769 - Flags: feedback+
Comment on attachment 8676769 [details] [diff] [review]
A possible fix

I, too, would prefer that we just skipped SizeShellTo if GetContentSize failed.  Though it's not really clear under what (normal) circumstances it would fail and what the behavior should really be in those circumstances...
Attachment #8676769 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/561f28136816
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in before you can comment on or make changes to this bug.