ash: fix multiple stack-use-after-scope issues with GetPrimaryDisplay use.

ash/system/web_notification/ash_popup_alignment_delegate_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ash/common/system/web_notification/ash_popup_alignment_delegate.h"
 
 #include <utility>
 #include <vector>
 
 #include "ash/common/shelf/wm_shelf.h"
@@ skipping 53 matching lines @@
     if (!delegate.get()) {
       alignment_delegate_.reset();
       return;
     }
     alignment_delegate_ = std::move(delegate);
     UpdateWorkArea(alignment_delegate_.get(),
                    display::Screen::GetScreen()->GetPrimaryDisplay());
   }
 
   Position GetPositionInDisplay(const gfx::Point& point) {
-    const gfx::Rect& work_area =
+    const gfx::Rect work_area =

[sky, 2017/01/18 23:33:45]

Why do you need the copy here? I can't see how this function ends up changing
the displays.

[krasin1, 2017/01/19 17:36:38]

What happens here (before the fix) is the following series of events:

1. display::Screen::GetScreen() returns Screen* pointer
2. Then GetPrimaryDisplay() returns Display value. This value is stored on the
stack and has the scope of just this line of the code. After that the compiler
is free to reuse the stack space (which it often does)
3. work_area() is called and returns a reference to a Display field. Once
Display goes out of scope (and destroyed), the reference saved in |work_area|
points to an already potentially reused memory, and not to what it meant to
refer to.
4. work_area is then used and AddressSanitizer with use-after-scope check
correctly points out to the issue.

A self contained example of that would be:

$ cat lala.cc
#include <stdio.h>

class Display {
 public:
  Display() : x_(12) { printf("Display\n"); }
  ~Display() { printf("~Display\n"); }

  int& GetX() { return x_; }

  static Display GetDisplay() { return Display(); }
 private:
  int x_;
};

int main(void) {
  const int& x = Display::GetDisplay().GetX();
  printf("x: %d\n", x);
  return 0;
}

$ clang++ lala.cc -o lala -std=c++14 -Wall -Werror && ./lala
Display
~Display
x: 12

As you can see, Display::GetDisplay() returns Display and then GetX returns a
reference to x_ inside the Display. We then access this reference, and as it's
shown by the debug prints, the access happens after the destructor ran.

Now, trying the same with use-after-scope check:

$ clang++ lala.cc -o lala -std=c++14 -g -fsanitize=address
-fsanitize-address-use-after-scope -Wall -Werror && ./lala
Display
~Display
=================================================================
==87088==ERROR: AddressSanitizer: stack-use-after-scope on address
0x7fff69f94ba0 at pc 0x00000051838e bp 0x7fff69f94b70 sp 0x7fff69f94b68
READ of size 4 at 0x7fff69f94ba0 thread T0
    #0 0x51838d in main /usr/local/google/home/krasin/play/scope/lala.cc:17:21
    #1 0x7fe63c319f44 in __libc_start_main
/build/eglibc-oGUzwX/eglibc-2.19/csu/libc-start.c:287
    #2 0x41adeb in _start
(/usr/local/google/home/krasin/play/scope/lala+0x41adeb)

Address 0x7fff69f94ba0 is located in stack of thread T0 at offset 32 in frame
    #0 0x51825f in main /usr/local/google/home/krasin/play/scope/lala.cc:15

  This frame has 1 object(s):
    [32, 36) 'ref.tmp:16' <== Memory access at offset 32 is inside this variable
HINT: this may be a false positive if your program uses some custom stack unwind
mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-use-after-scope
/usr/local/google/home/krasin/play/scope/lala.cc:17:21 in main

Absolutely the same thing happens in this test too, and the remedy is to either
save Display object into a local variable (that will have a larger scope), or
make |work_area| to store a value instead of a reference. I made a decision to
store a value, but let me know if you prefer the other way:

const Display display = display::Screen::GetScreen()->GetPrimaryDisplay();
const gfx::Rect& work_area = display.work_area();

(and the same in the other places)

         display::Screen::GetScreen()->GetPrimaryDisplay().work_area();
     const gfx::Point center_point = work_area.CenterPoint();
     if (work_area.x() > point.x() || work_area.y() > point.y() ||
         work_area.right() < point.x() || work_area.bottom() < point.y()) {
       return OUTSIDE;
     }
 
     if (center_point.x() < point.x())
       return (center_point.y() < point.y()) ? BOTTOM_RIGHT : TOP_RIGHT;
     else
@@ skipping 198 matching lines @@
   shelf->SetVirtualKeyboardBoundsForTesting(keyboard_bounds);
   int keyboard_baseline = alignment_delegate()->GetBaseLine();
   EXPECT_NE(baseline, keyboard_baseline);
   EXPECT_GT(keyboard_bounds.y(), keyboard_baseline);
 
   shelf->SetVirtualKeyboardBoundsForTesting(gfx::Rect());
   EXPECT_EQ(baseline, alignment_delegate()->GetBaseLine());
 }
 
 }  // namespace ash