Merge pull request #750 from atom/fix-leaking-webcontents

Handle window.open and <a target="..."> correctly

atom.gyp

@@ -30,6 +30,7 @@
       'atom/browser/api/lib/web-contents.coffee',
       'atom/browser/lib/chrome-extension.coffee',
       'atom/browser/lib/guest-view-manager.coffee',
+      'atom/browser/lib/guest-window-manager.coffee',
       'atom/browser/lib/init.coffee',
       'atom/browser/lib/objects-registry.coffee',
       'atom/browser/lib/rpc-server.coffee',

atom/browser/api/atom_api_web_contents.cc

@@ -88,7 +88,7 @@ bool WebContents::ShouldCreateWebContents(
     content::SessionStorageNamespace* session_storage_namespace) {
   base::ListValue args;
   args.AppendString(target_url.spec());
-  args.AppendString(partition_id);
+  args.AppendString(frame_name);
   Emit("new-window", args);
   return false;
 }
@@ -441,6 +441,7 @@ mate::ObjectTemplateBuilder WebContents::GetObjectTemplateBuilder(
         .SetMethod("_send", &WebContents::SendIPCMessage)
         .SetMethod("setAutoSize", &WebContents::SetAutoSize)
         .SetMethod("setAllowTransparency", &WebContents::SetAllowTransparency)
+        .SetMethod("isGuest", &WebContents::is_guest)
         .Build());
 
   return mate::ObjectTemplateBuilder(

atom/browser/api/atom_api_window.cc

@@ -82,6 +82,15 @@ void Window::OnPageTitleUpdated(bool* prevent_default,
   *prevent_default = Emit("page-title-updated", args);
 }
 
+void Window::WillCreatePopupWindow(const base::string16& frame_name,
+                                   const GURL& target_url,
+                                   const std::string& partition_id) {
+  base::ListValue args;
+  args.AppendString(target_url.spec());
+  args.AppendString(frame_name);
+  Emit("new-window", args);
+}
+
 void Window::WillCloseWindow(bool* prevent_default) {
   *prevent_default = Emit("close");
 }
@@ -122,6 +131,10 @@ void Window::Close() {
   window_->Close();
 }
 
+bool Window::IsClosed() {
+  return window_->IsClosed();
+}
+
 void Window::Focus() {
   window_->Focus(true);
 }
@@ -370,6 +383,7 @@ void Window::BuildPrototype(v8::Isolate* isolate,
   mate::ObjectTemplateBuilder(isolate, prototype)
       .SetMethod("destroy", &Window::Destroy)
       .SetMethod("close", &Window::Close)
+      .SetMethod("isClosed", &Window::IsClosed)
       .SetMethod("focus", &Window::Focus)
       .SetMethod("isFocused", &Window::IsFocused)
       .SetMethod("show", &Window::Show)

atom/browser/api/atom_api_window.h

@@ -43,19 +43,23 @@ class Window : public mate::EventEmitter,
   virtual ~Window();
 
   // Implementations of NativeWindowObserver:
-  virtual void OnPageTitleUpdated(bool* prevent_default,
-                                  const std::string& title) OVERRIDE;
-  virtual void WillCloseWindow(bool* prevent_default) OVERRIDE;
-  virtual void OnWindowClosed() OVERRIDE;
-  virtual void OnWindowBlur() OVERRIDE;
-  virtual void OnWindowFocus() OVERRIDE;
-  virtual void OnRendererUnresponsive() OVERRIDE;
-  virtual void OnRendererResponsive() OVERRIDE;
+  void OnPageTitleUpdated(bool* prevent_default,
+                          const std::string& title) override;
+  void WillCreatePopupWindow(const base::string16& frame_name,
+                             const GURL& target_url,
+                             const std::string& partition_id) override;
+  void WillCloseWindow(bool* prevent_default) override;
+  void OnWindowClosed() override;
+  void OnWindowBlur() override;
+  void OnWindowFocus() override;
+  void OnRendererUnresponsive() override;
+  void OnRendererResponsive() override;
 
  private:
   // APIs for NativeWindow.
   void Destroy();
   void Close();
+  bool IsClosed();
   void Focus();
   bool IsFocused();
   void Show();

atom/browser/api/event.cc

@@ -19,8 +19,7 @@ v8::Persistent<v8::ObjectTemplate> template_;
 
 Event::Event()
     : sender_(NULL),
-      message_(NULL),
-      prevent_default_(false) {
+      message_(NULL) {
 }
 
 Event::~Event() {
@@ -52,8 +51,9 @@ void Event::WebContentsDestroyed() {
   message_ = NULL;
 }
 
-void Event::PreventDefault() {
-  prevent_default_ = true;
+void Event::PreventDefault(v8::Isolate* isolate) {
+  GetWrapper(isolate)->Set(StringToV8(isolate, "defaultPrevented"),
+                           v8::True(isolate));
 }
 
 bool Event::SendReply(const base::string16& json) {

atom/browser/api/event.h

@@ -24,14 +24,11 @@ class Event : public Wrappable,
   void SetSenderAndMessage(content::WebContents* sender, IPC::Message* message);
 
   // event.PreventDefault().
-  void PreventDefault();
+  void PreventDefault(v8::Isolate* isolate);
 
   // event.sendReply(json), used for replying synchronous message.
   bool SendReply(const base::string16& json);
 
-  // Whether event.preventDefault() is called.
-  bool prevent_default() const { return prevent_default_; }
-
  protected:
   Event();
   virtual ~Event();
@@ -47,8 +44,6 @@ class Event : public Wrappable,
   content::WebContents* sender_;
   IPC::Message* message_;
 
-  bool prevent_default_;
-
   DISALLOW_COPY_AND_ASSIGN(Event);
 };
 

atom/browser/api/event_emitter.cc

@@ -98,17 +98,7 @@ bool EventEmitter::Emit(v8::Isolate* isolate,
   node::MakeCallback(isolate, GetWrapper(isolate), "emit", args.size(),
                      &args[0]);
 
-  if (use_native_event) {
-    Handle<Event> native_event;
-    if (ConvertFromV8(isolate, event, &native_event))
-      return native_event->prevent_default();
-  }
-
-  v8::Handle<v8::Value> prevent_default =
-      event->GetHiddenValue(StringToSymbol(isolate, "prevent_default"));
-  if (prevent_default.IsEmpty())
-    return false;
-  return prevent_default->BooleanValue();
+  return event->Get(StringToV8(isolate, "defaultPrevented"))->BooleanValue();
 }
 
 }  // namespace mate

atom/browser/api/lib/browser-window.coffee

@@ -1,6 +1,7 @@
 EventEmitter = require('events').EventEmitter
 IDWeakMap = require 'id-weak-map'
 app = require 'app'
+ipc = require 'ipc'
 wrapWebContents = require('web-contents').wrap
 
 BrowserWindow = process.atomBinding('window').BrowserWindow
@@ -23,6 +24,12 @@ BrowserWindow::_init = ->
     value: BrowserWindow.windows.add(this)
     enumerable: true
 
+  # Make new windows requested by links behave like "window.open"
+  @on 'new-window', (event, url, frameName) =>
+    event.sender = @webContents
+    options = show: true, width: 800, height: 600
+    ipc.emit 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_OPEN', event, url, frameName, options
+
   # Remove the window from weak map immediately when it's destroyed, since we
   # could be iterating windows before GC happened.
   @once 'closed', =>

atom/browser/lib/guest-window-manager.coffee

@@ -0,0 +1,53 @@
+ipc = require 'ipc'
+BrowserWindow = require 'browser-window'
+
+frameToGuest = {}
+
+# Create a new guest created by |embedder| with |options|.
+createGuest = (embedder, url, frameName, options) ->
+  guest = frameToGuest[frameName]
+  if frameName and guest?
+    guest.loadUrl url
+    return guest.id
+
+  guest = new BrowserWindow(options)
+  guest.loadUrl url
+
+  # When |embedder| is destroyed we should also destroy attached guest, and if
+  # guest is closed by user then we should prevent |embedder| from double
+  # closing guest.
+  closedByEmbedder = ->
+    guest.removeListener 'closed', closedByUser
+    guest.destroy() unless guest.isClosed()
+  closedByUser = ->
+    embedder.removeListener 'render-view-deleted', closedByEmbedder
+  embedder.once 'render-view-deleted', closedByEmbedder
+  guest.once 'closed', closedByUser
+
+  if frameName
+    frameToGuest[frameName] = guest
+    guest.frameName = frameName
+    guest.once 'closed', ->
+      delete frameToGuest[frameName]
+
+  guest.id
+
+# Routed window.open messages.
+ipc.on 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_OPEN', (event, args...) ->
+  event.sender.emit 'new-window', event, args...
+  if event.sender.isGuest() or event.defaultPrevented
+    event.returnValue = null
+  else
+    event.returnValue = createGuest event.sender, args...
+
+ipc.on 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', (event, guestId) ->
+  return unless BrowserWindow.windows.has guestId
+  BrowserWindow.windows.get(guestId).destroy()
+
+ipc.on 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_METHOD', (event, guestId, method, args...) ->
+  return unless BrowserWindow.windows.has guestId
+  BrowserWindow.windows.get(guestId)[method] args...
+
+ipc.on 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', (event, guestId, method, args...) ->
+  return unless BrowserWindow.windows.has guestId
+  BrowserWindow.windows.get(guestId).webContents?[method] args...

atom/browser/lib/init.coffee

@@ -58,10 +58,11 @@ process.once 'BIND_DONE', ->
     process.emit 'exit'
 
   # Load the RPC server.
-  require './rpc-server.js'
+  require './rpc-server'
 
   # Load the guest view manager.
-  require './guest-view-manager.js'
+  require './guest-view-manager'
+  require './guest-window-manager'
 
   # Now we try to load app's package.json.
   packageJson = null

atom/browser/native_window.cc

@@ -423,6 +423,22 @@ void NativeWindow::NotifyWindowFocus() {
   FOR_EACH_OBSERVER(NativeWindowObserver, observers_, OnWindowFocus());
 }
 
+bool NativeWindow::ShouldCreateWebContents(
+    content::WebContents* web_contents,
+    int route_id,
+    WindowContainerType window_container_type,
+    const base::string16& frame_name,
+    const GURL& target_url,
+    const std::string& partition_id,
+    content::SessionStorageNamespace* session_storage_namespace) {
+  FOR_EACH_OBSERVER(NativeWindowObserver,
+                    observers_,
+                    WillCreatePopupWindow(frame_name,
+                                          target_url,
+                                          partition_id));
+  return false;
+}
+
 // In atom-shell all reloads and navigations started by renderer process would
 // be redirected to this method, so we can have precise control of how we
 // would open the url (in our case, is to restart the renderer process). See

atom/browser/native_window.h

@@ -214,43 +214,50 @@ class NativeWindow : public brightray::DefaultWebContentsDelegate,
       const std::vector<DraggableRegion>& regions) = 0;
 
   // Implementations of content::WebContentsDelegate.
-  virtual content::WebContents* OpenURLFromTab(
+  bool ShouldCreateWebContents(
+      content::WebContents* web_contents,
+      int route_id,
+      WindowContainerType window_container_type,
+      const base::string16& frame_name,
+      const GURL& target_url,
+      const std::string& partition_id,
+      content::SessionStorageNamespace* session_storage_namespace) override;
+  content::WebContents* OpenURLFromTab(
       content::WebContents* source,
-      const content::OpenURLParams& params) OVERRIDE;
-  virtual content::JavaScriptDialogManager*
-      GetJavaScriptDialogManager() OVERRIDE;
-  virtual void BeforeUnloadFired(content::WebContents* tab,
-                                 bool proceed,
-                                 bool* proceed_to_fire_unload) OVERRIDE;
-  virtual void RequestToLockMouse(content::WebContents* web_contents,
-                                  bool user_gesture,
-                                  bool last_unlocked_by_target) OVERRIDE;
-  virtual bool CanOverscrollContent() const OVERRIDE;
-  virtual void ActivateContents(content::WebContents* contents) OVERRIDE;
-  virtual void DeactivateContents(content::WebContents* contents) OVERRIDE;
-  virtual void MoveContents(content::WebContents* source,
-                            const gfx::Rect& pos) OVERRIDE;
-  virtual void CloseContents(content::WebContents* source) OVERRIDE;
-  virtual bool IsPopupOrPanel(
-      const content::WebContents* source) const OVERRIDE;
-  virtual void RendererUnresponsive(content::WebContents* source) OVERRIDE;
-  virtual void RendererResponsive(content::WebContents* source) OVERRIDE;
+      const content::OpenURLParams& params) override;
+  content::JavaScriptDialogManager* GetJavaScriptDialogManager() override;
+  void BeforeUnloadFired(content::WebContents* tab,
+                         bool proceed,
+                         bool* proceed_to_fire_unload) override;
+  void RequestToLockMouse(content::WebContents* web_contents,
+                          bool user_gesture,
+                          bool last_unlocked_by_target) override;
+  bool CanOverscrollContent() const override;
+  void ActivateContents(content::WebContents* contents) override;
+  void DeactivateContents(content::WebContents* contents) override;
+  void MoveContents(content::WebContents* source,
+                    const gfx::Rect& pos) override;
+  void CloseContents(content::WebContents* source) override;
+  bool IsPopupOrPanel(
+      const content::WebContents* source) const override;
+  void RendererUnresponsive(content::WebContents* source) override;
+  void RendererResponsive(content::WebContents* source) override;
 
   // Implementations of content::WebContentsObserver.
-  virtual void BeforeUnloadFired(const base::TimeTicks& proceed_time) OVERRIDE;
-  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  void BeforeUnloadFired(const base::TimeTicks& proceed_time) override;
+  bool OnMessageReceived(const IPC::Message& message) override;
 
   // Implementations of content::NotificationObserver.
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
+  void Observe(int type,
+               const content::NotificationSource& source,
+               const content::NotificationDetails& details) override;
 
   // Implementations of brightray::InspectableWebContentsDelegate.
-  virtual void DevToolsSaveToFile(const std::string& url,
-                                  const std::string& content,
-                                  bool save_as) OVERRIDE;
-  virtual void DevToolsAppendToFile(const std::string& url,
-                                    const std::string& content) OVERRIDE;
+  void DevToolsSaveToFile(const std::string& url,
+                          const std::string& content,
+                          bool save_as) override;
+  void DevToolsAppendToFile(const std::string& url,
+                            const std::string& content) override;
 
   // Whether window has standard frame.
   bool has_frame_;

atom/browser/native_window_observer.h

@@ -7,6 +7,9 @@
 
 #include <string>
 
+#include "base/strings/string16.h"
+#include "url/gurl.h"
+
 namespace atom {
 
 class NativeWindowObserver {
@@ -17,6 +20,11 @@ class NativeWindowObserver {
   virtual void OnPageTitleUpdated(bool* prevent_default,
                                   const std::string& title) {}
 
+  // Called when the web page in window wants to create a popup window.
+  virtual void WillCreatePopupWindow(const base::string16& frame_name,
+                                     const GURL& target_url,
+                                     const std::string& partition_id) {}
+
   // Called when the window is gonna closed.
   virtual void WillCloseWindow(bool* prevent_default) {}
 

atom/renderer/lib/guest-view-internal.coffee

@@ -10,7 +10,7 @@ WEB_VIEW_EVENTS =
   'did-stop-loading': []
   'did-get-redirect-request': ['oldUrl', 'newUrl', 'isMainFrame']
   'console-message': ['level', 'message', 'line', 'sourceId']
-  'new-window': ['url', 'partitionId']
+  'new-window': ['url', 'frameName']
   'close': []
   'crashed': []
   'destroyed': []

atom/renderer/lib/override.coffee

@@ -1,34 +1,48 @@
 process = global.process
+ipc = require 'ipc'
 remote = require 'remote'
 
+# Window object returned by "window.open".
+class FakeWindow
+  constructor: (@guestId) ->
+
+  close: ->
+    ipc.send 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', @guestId
+
+  focus: ->
+    ipc.send 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_METHOD', @guestId, 'focus'
+
+  blur: ->
+    ipc.send 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_METHOD', @guestId, 'blur'
+
+  eval: (args...) ->
+    ipc.send 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', @guestId, 'executeJavaScript', args...
+
 unless process.guestInstanceId?
-  # Override default window.close, see:
+  # Override default window.close.
   window.close = ->
     remote.getCurrentWindow().close()
 
-  # Override default window.open.
-  window.open = (url, name, features) ->
-    options = {}
-    for feature in features.split ','
-      [name, value] = feature.split '='
-      options[name] =
-        if value is 'yes'
-          true
-        else if value is 'no'
-          false
-        else
-          value
+# Make the browser window or guest view emit "new-window" event.
+window.open = (url, frameName='', features='') ->
+  options = {}
+  for feature in features.split ','
+    [name, value] = feature.split '='
+    options[name] =
+      if value is 'yes'
+        true
+      else if value is 'no'
+        false
+      else
+        value
+  options.x ?= options.left
+  options.y ?= options.top
+  options.title ?= name
+  options.width ?= 800
+  options.height ?= 600
 
-    options.x ?= options.left
-    options.y ?= options.top
-    options.title ?= name
-    options.width ?= 800
-    options.height ?= 600
-
-    BrowserWindow = require('remote').require 'browser-window'
-    browser = new BrowserWindow options
-    browser.loadUrl url
-    browser
+  guestId = ipc.sendSync 'ATOM_SHELL_GUEST_WINDOW_MANAGER_WINDOW_OPEN', url, frameName, options
+  new FakeWindow(guestId)
 
 # Use the dialog API to implement alert().
 window.alert = (message, title='') ->

docs/api/browser-window.md

@@ -546,6 +546,21 @@ Corresponds to the points in time when the spinner of the tab stops spinning.
 
 Emitted when a redirect was received while requesting a resource.
 
+### Event: 'new-window'
+
+* `event` Event
+* `url` String
+* `frameName` String
+* `options` Object
+
+Emitted when the page requested to open a new window for `url`. It could be
+requested by `window.open` or a external link like `<a target='_blank'>`.
+
+By default a new `BrowserWindow` will be created for `url` called, and a proxy
+will be returned to `window.open` to let you have limited control of it.
+
+Calling `event.preventDefault()` can prevent creating new windows.
+
 ### Event: 'crashed'
 
 Emitted when the renderer process is crashed.

docs/api/web-view-tag.md

@@ -237,7 +237,7 @@ webview.addEventListener('console-message', function(e) {
 ### new-window
 
 * `url` String
-* `partitionId` String
+* `frameName` String
 
 Fired when the guest page attempts to open a new browser window.
 

spec/api-browser-window-spec.coffee

@@ -156,3 +156,20 @@ describe 'browser-window module', ->
       w.on 'onbeforeunload', ->
         done()
       w.loadUrl 'file://' + path.join(fixtures, 'api', 'close-beforeunload-empty-string.html')
+
+  describe 'new-window event', ->
+    it 'emits when window.open is called', (done) ->
+      w.webContents.once 'new-window', (e, url, frameName) ->
+        e.preventDefault()
+        assert.equal url, 'http://host'
+        assert.equal frameName, 'host'
+        done()
+      w.loadUrl "file://#{fixtures}/pages/window-open.html"
+
+    it 'emits when link with target is called', (done) ->
+      w.webContents.once 'new-window', (e, url, frameName) ->
+        e.preventDefault()
+        assert.equal url, 'http://host/'
+        assert.equal frameName, 'target'
+        done()
+      w.loadUrl "file://#{fixtures}/pages/target-name.html"

spec/chromium-spec.coffee

@@ -32,10 +32,10 @@ describe 'chromium feature', ->
       assert.notEqual navigator.language, ''
 
   describe 'window.open', ->
-    it 'returns a BrowserWindow object', ->
+    it 'returns a FakeWindow object', ->
       b = window.open 'about:blank', 'test', 'show=no'
-      assert.equal b.constructor.name, 'BrowserWindow'
-      b.destroy()
+      assert.equal b.constructor.name, 'FakeWindow'
+      b.close()
 
   describe 'creating a Uint8Array under browser side', ->
     it 'does not crash', ->

spec/fixtures/pages/target-name.html

@@ -0,0 +1,13 @@
+<html>
+<body>
+<a id="a", href="http://host" target="target">link</a>
+<script type="text/javascript" charset="utf-8">
+  var event = new MouseEvent('click', {
+    'view': window,
+    'bubbles': true,
+    'cancelable': true
+  });
+  document.getElementById('a').dispatchEvent(event);
+</script>
+</body>
+</html>

spec/fixtures/pages/window-open.html

@@ -0,0 +1,7 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  window.open('http://host', 'host');
+</script>
+</body>
+</html>

spec/node-spec.coffee

@@ -96,13 +96,14 @@ describe 'node feature', ->
             setImmediate done
 
   describe 'net.connect', ->
-    it 'emit error when connect to a socket path without listeners', (done) ->
-      return done() if process.platform is 'win32'
+    return unless process.platform is 'darwin'
 
+    it 'emit error when connect to a socket path without listeners', (done) ->
       socketPath = path.join os.tmpdir(), 'atom-shell-test.sock'
       script = path.join(fixtures, 'module', 'create_socket.js')
       child = child_process.fork script, [socketPath]
-      child.on 'exit', ->
+      child.on 'exit', (code) ->
+        assert.equal code, 0
         client = require('net').connect socketPath
         client.on 'error', (error) ->
           assert.equal error.code, 'ECONNREFUSED'

spec/webview-spec.coffee

@@ -46,3 +46,20 @@ describe '<webview> tag', ->
       webview.setAttribute 'nodeintegration', 'on'
       webview.src = "file://#{fixtures}/pages/d.html"
       document.body.appendChild webview
+
+  describe 'new-window event', ->
+    it 'emits when window.open is called', (done) ->
+      webview.addEventListener 'new-window', (e) ->
+        assert.equal e.url, 'http://host'
+        assert.equal e.frameName, 'host'
+        done()
+      webview.src = "file://#{fixtures}/pages/window-open.html"
+      document.body.appendChild webview
+
+    it 'emits when link with target is called', (done) ->
+      webview.addEventListener 'new-window', (e) ->
+        assert.equal e.url, 'http://host/'
+        assert.equal e.frameName, 'target'
+        done()
+      webview.src = "file://#{fixtures}/pages/target-name.html"
+      document.body.appendChild webview

vendor/brightray

@@ -1 +1 @@
-Subproject commit 319c63da618f5fdff38f6a65c452f3802b8756d1
+Subproject commit ba89e08f8dcec06a65068c6c959431e7914fc00d