Merge branch 'master' into native-window-open

spec/api-browser-window-spec.js

@@ -692,7 +692,7 @@ describe('BrowserWindow module', function () {
     })
   })
 
-  describe('"title-bar-style" option', function () {
+  describe('"titleBarStyle" option', function () {
     if (process.platform !== 'darwin') {
       return
     }
@@ -772,6 +772,20 @@ describe('BrowserWindow module', function () {
     })
   })
 
+  describe('"tabbingIdentifier" option', function () {
+    it('can be set on a window', function () {
+      w.destroy()
+      w = new BrowserWindow({
+        tabbingIdentifier: 'group1'
+      })
+      w.destroy()
+      w = new BrowserWindow({
+        tabbingIdentifier: 'group2',
+        frame: false
+      })
+    })
+  })
+
   describe('"web-preferences" option', function () {
     afterEach(function () {
       ipcMain.removeAllListeners('answer')

spec/api-crash-reporter-spec.js

@@ -1,5 +1,6 @@
 const assert = require('assert')
 const childProcess = require('child_process')
+const fs = require('fs')
 const http = require('http')
 const multiparty = require('multiparty')
 const path = require('path')
@@ -11,85 +12,183 @@ const {remote} = require('electron')
 const {app, BrowserWindow, crashReporter} = remote.require('electron')
 
 describe('crashReporter module', function () {
-  var fixtures = path.resolve(__dirname, 'fixtures')
-  var w = null
-  var originalTempDirectory = null
-  var tempDirectory = null
-
-  beforeEach(function () {
-    w = new BrowserWindow({
-      show: false
-    })
-    tempDirectory = temp.mkdirSync('electronCrashReporterSpec-')
-    originalTempDirectory = app.getPath('temp')
-    app.setPath('temp', tempDirectory)
-  })
-
-  afterEach(function () {
-    app.setPath('temp', originalTempDirectory)
-    return closeWindow(w).then(function () { w = null })
-  })
-
   if (process.mas) {
     return
   }
+  var fixtures = path.resolve(__dirname, 'fixtures')
+  const generateSpecs = (description, browserWindowOpts) => {
+    describe(description, function () {
+      var w = null
+      var originalTempDirectory = null
+      var tempDirectory = null
 
-  it('should send minidump when renderer crashes', function (done) {
-    if (process.env.APPVEYOR === 'True') return done()
-    if (process.env.TRAVIS === 'true') return done()
+      beforeEach(function () {
+        w = new BrowserWindow(Object.assign({
+          show: false
+        }, browserWindowOpts))
+        tempDirectory = temp.mkdirSync('electronCrashReporterSpec-')
+        originalTempDirectory = app.getPath('temp')
+        app.setPath('temp', tempDirectory)
+      })
 
-    this.timeout(120000)
+      afterEach(function () {
+        app.setPath('temp', originalTempDirectory)
+        return closeWindow(w).then(function () { w = null })
+      })
 
-    startServer({
-      callback (port) {
-        const crashUrl = url.format({
-          protocol: 'file',
-          pathname: path.join(fixtures, 'api', 'crash.html'),
-          search: '?port=' + port
+      it('should send minidump when renderer crashes', function (done) {
+        if (process.env.APPVEYOR === 'True') return done()
+        if (process.env.TRAVIS === 'true') return done()
+
+        this.timeout(120000)
+
+        startServer({
+          callback (port) {
+            const crashUrl = url.format({
+              protocol: 'file',
+              pathname: path.join(fixtures, 'api', 'crash.html'),
+              search: '?port=' + port
+            })
+            w.loadURL(crashUrl)
+          },
+          processType: 'renderer',
+          done: done
         })
-        w.loadURL(crashUrl)
-      },
-      processType: 'renderer',
-      done: done
-    })
-  })
+      })
 
-  it('should send minidump when node processes crash', function (done) {
-    if (process.env.APPVEYOR === 'True') return done()
-    if (process.env.TRAVIS === 'true') return done()
+      it('should send minidump when node processes crash', function (done) {
+        if (process.env.APPVEYOR === 'True') return done()
+        if (process.env.TRAVIS === 'true') return done()
 
-    this.timeout(120000)
+        this.timeout(120000)
 
-    startServer({
-      callback (port) {
-        const crashesDir = path.join(app.getPath('temp'), `${app.getName()} Crashes`)
-        const version = app.getVersion()
-        const crashPath = path.join(fixtures, 'module', 'crash.js')
-        childProcess.fork(crashPath, [port, version, crashesDir], {silent: true})
-      },
-      processType: 'browser',
-      done: done
-    })
-  })
-
-  it('should send minidump with updated extra parameters', function (done) {
-    if (process.env.APPVEYOR === 'True') return done()
-    if (process.env.TRAVIS === 'true') return done()
-
-    this.timeout(10000)
-
-    startServer({
-      callback (port) {
-        const crashUrl = url.format({
-          protocol: 'file',
-          pathname: path.join(fixtures, 'api', 'crash-restart.html'),
-          search: '?port=' + port
+        startServer({
+          callback (port) {
+            const crashesDir = path.join(app.getPath('temp'), `${app.getName()} Crashes`)
+            const version = app.getVersion()
+            const crashPath = path.join(fixtures, 'module', 'crash.js')
+            childProcess.fork(crashPath, [port, version, crashesDir], {silent: true})
+          },
+          processType: 'browser',
+          done: done
         })
-        w.loadURL(crashUrl)
-      },
-      processType: 'renderer',
-      done: done
+      })
+
+      it('should not send minidump if uploadToServer is false', function (done) {
+        this.timeout(120000)
+
+        if (process.platform === 'darwin') {
+          crashReporter.setUploadToServer(false)
+        }
+
+        let server
+        let dumpFile
+        let crashesDir
+        const testDone = (uploaded) => {
+          if (uploaded) {
+            return done(new Error('fail'))
+          }
+          server.close()
+          if (process.platform === 'darwin') {
+            crashReporter.setUploadToServer(true)
+          }
+          assert(fs.existsSync(dumpFile))
+          fs.unlinkSync(dumpFile)
+          done()
+        }
+
+        let pollInterval
+        const pollDumpFile = () => {
+          fs.readdir(crashesDir, (err, files) => {
+            if (err) {
+              return
+            }
+            const dumps = files.filter((file) => /\.dmp$/.test(file))
+            if (!dumps.length) {
+              return
+            }
+            assert.equal(1, dumps.length)
+            dumpFile = path.join(crashesDir, dumps[0])
+            clearInterval(pollInterval)
+            // dump file should not be deleted when not uploading, so we wait
+            // 500 ms and assert it still exists in `testDone`
+            setTimeout(testDone, 500)
+          })
+        }
+
+        remote.ipcMain.once('set-crash-directory', (event, dir) => {
+          if (process.platform === 'linux') {
+            crashesDir = dir
+          } else {
+            crashesDir = crashReporter.getCrashesDirectory()
+            if (process.platform === 'darwin') {
+              // crashpad uses an extra subdirectory
+              crashesDir = path.join(crashesDir, 'completed')
+            }
+          }
+
+          // Before starting, remove all dump files in the crash directory.
+          // This is required because:
+          // - mac crashpad not seem to allow changing the crash directory after
+          //   the first "start" call.
+          // - Other tests in this suite may leave dumps there.
+          // - We want to verify in `testDone` that a dump file is created and
+          //   not deleted.
+          fs.readdir(crashesDir, (err, files) => {
+            if (!err) {
+              for (const file of files) {
+                if (/\.dmp$/.test(file)) {
+                  fs.unlinkSync(path.join(crashesDir, file))
+                }
+              }
+            }
+            event.returnValue = null  // allow the renderer to crash
+            pollInterval = setInterval(pollDumpFile, 100)
+          })
+        })
+
+        server = startServer({
+          callback (port) {
+            const crashUrl = url.format({
+              protocol: 'file',
+              pathname: path.join(fixtures, 'api', 'crash.html'),
+              search: `?port=${port}&skipUpload=1`
+            })
+            w.loadURL(crashUrl)
+          },
+          processType: 'renderer',
+          done: testDone.bind(null, true)
+        })
+      })
+
+      it('should send minidump with updated extra parameters', function (done) {
+        if (process.env.APPVEYOR === 'True') return done()
+        if (process.env.TRAVIS === 'true') return done()
+
+        this.timeout(10000)
+
+        startServer({
+          callback (port) {
+            const crashUrl = url.format({
+              protocol: 'file',
+              pathname: path.join(fixtures, 'api', 'crash-restart.html'),
+              search: '?port=' + port
+            })
+            w.loadURL(crashUrl)
+          },
+          processType: 'renderer',
+          done: done
+        })
+      })
     })
+  }
+
+  generateSpecs('without sandbox', {})
+  generateSpecs('with sandbox ', {
+    webPreferences: {
+      sandbox: true,
+      preload: path.join(fixtures, 'module', 'preload-sandbox.js')
+    }
   })
 
   describe('.start(options)', function () {
@@ -204,4 +303,5 @@ const startServer = ({callback, processType, done}) => {
     }
     callback(port)
   })
+  return server
 }

spec/api-ipc-spec.js

@@ -187,6 +187,18 @@ describe('ipc module', function () {
       property.property = 1127
     })
 
+    it('rethrows errors getting/setting properties', function () {
+      const foo = remote.require(path.join(fixtures, 'module', 'error-properties.js'))
+
+      assert.throws(function () {
+        foo.bar
+      }, /getting error/)
+
+      assert.throws(function () {
+        foo.bar = 'test'
+      }, /setting error/)
+    })
+
     it('can construct an object from its member', function () {
       var call = remote.require(path.join(fixtures, 'module', 'call.js'))
       var obj = new call.constructor()

spec/api-menu-spec.js

@@ -455,6 +455,57 @@ describe('menu module', function () {
     })
   })
 
+  describe('MenuItem editMenu', function () {
+    it('includes a default submenu layout when submenu is empty', function () {
+      var item = new MenuItem({role: 'editMenu'})
+      assert.equal(item.label, 'Edit')
+      assert.equal(item.submenu.items[0].role, 'undo')
+      assert.equal(item.submenu.items[1].role, 'redo')
+      assert.equal(item.submenu.items[2].type, 'separator')
+      assert.equal(item.submenu.items[3].role, 'cut')
+      assert.equal(item.submenu.items[4].role, 'copy')
+      assert.equal(item.submenu.items[5].role, 'paste')
+
+      if (process.platform === 'darwin') {
+        assert.equal(item.submenu.items[6].role, 'pasteandmatchstyle')
+        assert.equal(item.submenu.items[7].role, 'delete')
+        assert.equal(item.submenu.items[8].role, 'selectall')
+      }
+
+      if (process.platform === 'win32') {
+        assert.equal(item.submenu.items[6].role, 'delete')
+        assert.equal(item.submenu.items[7].type, 'separator')
+        assert.equal(item.submenu.items[8].role, 'selectall')
+      }
+    })
+
+    it('overrides default layout when submenu is specified', function () {
+      var item = new MenuItem({role: 'editMenu', submenu: [{role: 'close'}]})
+      assert.equal(item.label, 'Edit')
+      assert.equal(item.submenu.items[0].role, 'close')
+    })
+  })
+
+  describe('MenuItem windowMenu', function () {
+    it('includes a default submenu layout when submenu is empty', function () {
+      var item = new MenuItem({role: 'windowMenu'})
+      assert.equal(item.label, 'Window')
+      assert.equal(item.submenu.items[0].role, 'minimize')
+      assert.equal(item.submenu.items[1].role, 'close')
+
+      if (process.platform === 'darwin') {
+        assert.equal(item.submenu.items[2].type, 'separator')
+        assert.equal(item.submenu.items[3].role, 'front')
+      }
+    })
+
+    it('overrides default layout when submenu is specified', function () {
+      var item = new MenuItem({role: 'windowMenu', submenu: [{role: 'copy'}]})
+      assert.equal(item.label, 'Window')
+      assert.equal(item.submenu.items[0].role, 'copy')
+    })
+  })
+
   describe('MenuItem with custom properties in constructor', function () {
     it('preserves the custom properties', function () {
       var template = [{

spec/api-net-spec.js

@@ -364,6 +364,49 @@ describe('net module', function () {
       urlRequest.end()
     })
 
+    it('should be able to set a non-string object as a header value', function (done) {
+      const requestUrl = '/requestUrl'
+      const customHeaderName = 'Some-Integer-Value'
+      const customHeaderValue = 900
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case requestUrl:
+            assert.equal(request.headers[customHeaderName.toLowerCase()],
+              customHeaderValue.toString())
+            response.statusCode = 200
+            response.statusMessage = 'OK'
+            response.end()
+            break
+          default:
+            assert.equal(request.url, requestUrl)
+        }
+      })
+      const urlRequest = net.request({
+        method: 'GET',
+        url: `${server.url}${requestUrl}`
+      })
+      urlRequest.on('response', function (response) {
+        const statusCode = response.statusCode
+        assert.equal(statusCode, 200)
+        response.pause()
+        response.on('end', function () {
+          done()
+        })
+        response.resume()
+      })
+      urlRequest.setHeader(customHeaderName, customHeaderValue)
+      assert.equal(urlRequest.getHeader(customHeaderName),
+        customHeaderValue)
+      assert.equal(urlRequest.getHeader(customHeaderName.toLowerCase()),
+        customHeaderValue)
+      urlRequest.write('')
+      assert.equal(urlRequest.getHeader(customHeaderName),
+        customHeaderValue)
+      assert.equal(urlRequest.getHeader(customHeaderName.toLowerCase()),
+        customHeaderValue)
+      urlRequest.end()
+    })
+
     it('should not be able to set a custom HTTP request header after first write', function (done) {
       const requestUrl = '/requestUrl'
       const customHeaderName = 'Some-Custom-Header-Name'
@@ -906,6 +949,217 @@ describe('net module', function () {
       urlRequest.end()
     })
 
+    it('should throw if given an invalid redirect mode', function () {
+      const requestUrl = '/requestUrl'
+      const options = {
+        url: `${server.url}${requestUrl}`,
+        redirect: 'custom'
+      }
+      assert.throws(function () {
+        net.request(options)
+      }, 'redirect mode should be one of follow, error or manual')
+    })
+
+    it('should throw when calling getHeader without a name', function () {
+      assert.throws(function () {
+        net.request({url: `${server.url}/requestUrl`}).getHeader()
+      }, /`name` is required for getHeader\(name\)\./)
+
+      assert.throws(function () {
+        net.request({url: `${server.url}/requestUrl`}).getHeader(null)
+      }, /`name` is required for getHeader\(name\)\./)
+    })
+
+    it('should throw when calling removeHeader without a name', function () {
+      assert.throws(function () {
+        net.request({url: `${server.url}/requestUrl`}).removeHeader()
+      }, /`name` is required for removeHeader\(name\)\./)
+
+      assert.throws(function () {
+        net.request({url: `${server.url}/requestUrl`}).removeHeader(null)
+      }, /`name` is required for removeHeader\(name\)\./)
+    })
+
+    it('should follow redirect when no redirect mode is provided', function (done) {
+      const requestUrl = '/301'
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case '/301':
+            response.statusCode = '301'
+            response.setHeader('Location', '/200')
+            response.end()
+            break
+          case '/200':
+            response.statusCode = '200'
+            response.end()
+            break
+          default:
+            assert(false)
+        }
+      })
+      const urlRequest = net.request({
+        url: `${server.url}${requestUrl}`
+      })
+      urlRequest.on('response', function (response) {
+        assert.equal(response.statusCode, 200)
+        done()
+      })
+      urlRequest.end()
+    })
+
+    it('should follow redirect chain when no redirect mode is provided', function (done) {
+      const requestUrl = '/redirectChain'
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case '/redirectChain':
+            response.statusCode = '301'
+            response.setHeader('Location', '/301')
+            response.end()
+            break
+          case '/301':
+            response.statusCode = '301'
+            response.setHeader('Location', '/200')
+            response.end()
+            break
+          case '/200':
+            response.statusCode = '200'
+            response.end()
+            break
+          default:
+            assert(false)
+        }
+      })
+      const urlRequest = net.request({
+        url: `${server.url}${requestUrl}`
+      })
+      urlRequest.on('response', function (response) {
+        assert.equal(response.statusCode, 200)
+        done()
+      })
+      urlRequest.end()
+    })
+
+    it('should not follow redirect when mode is error', function (done) {
+      const requestUrl = '/301'
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case '/301':
+            response.statusCode = '301'
+            response.setHeader('Location', '/200')
+            response.end()
+            break
+          case '/200':
+            response.statusCode = '200'
+            response.end()
+            break
+          default:
+            assert(false)
+        }
+      })
+      const urlRequest = net.request({
+        url: `${server.url}${requestUrl}`,
+        redirect: 'error'
+      })
+      urlRequest.on('error', function (error) {
+        assert.equal(error.message, 'Request cannot follow redirect with the current redirect mode')
+      })
+      urlRequest.on('close', function () {
+        done()
+      })
+      urlRequest.end()
+    })
+
+    it('should allow follow redirect when mode is manual', function (done) {
+      const requestUrl = '/redirectChain'
+      let redirectCount = 0
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case '/redirectChain':
+            response.statusCode = '301'
+            response.setHeader('Location', '/301')
+            response.end()
+            break
+          case '/301':
+            response.statusCode = '301'
+            response.setHeader('Location', '/200')
+            response.end()
+            break
+          case '/200':
+            response.statusCode = '200'
+            response.end()
+            break
+          default:
+            assert(false)
+        }
+      })
+      const urlRequest = net.request({
+        url: `${server.url}${requestUrl}`,
+        redirect: 'manual'
+      })
+      urlRequest.on('response', function (response) {
+        assert.equal(response.statusCode, 200)
+        assert.equal(redirectCount, 2)
+        done()
+      })
+      urlRequest.on('redirect', function (status, method, url) {
+        if (url === `${server.url}/301` || url === `${server.url}/200`) {
+          redirectCount += 1
+          urlRequest.followRedirect()
+        }
+      })
+      urlRequest.end()
+    })
+
+    it('should allow cancelling redirect when mode is manual', function (done) {
+      const requestUrl = '/redirectChain'
+      let redirectCount = 0
+      server.on('request', function (request, response) {
+        switch (request.url) {
+          case '/redirectChain':
+            response.statusCode = '301'
+            response.setHeader('Location', '/redirect/1')
+            response.end()
+            break
+          case '/redirect/1':
+            response.statusCode = '200'
+            response.setHeader('Location', '/redirect/2')
+            response.end()
+            break
+          case '/redirect/2':
+            response.statusCode = '200'
+            response.end()
+            break
+          default:
+            assert(false)
+        }
+      })
+      const urlRequest = net.request({
+        url: `${server.url}${requestUrl}`,
+        redirect: 'manual'
+      })
+      urlRequest.on('response', function (response) {
+        assert.equal(response.statusCode, 200)
+        response.pause()
+        response.on('data', function (chunk) {
+        })
+        response.on('end', function () {
+          urlRequest.abort()
+        })
+        response.resume()
+      })
+      urlRequest.on('close', function () {
+        assert.equal(redirectCount, 1)
+        done()
+      })
+      urlRequest.on('redirect', function (status, method, url) {
+        if (url === `${server.url}/redirect/1`) {
+          redirectCount += 1
+          urlRequest.followRedirect()
+        }
+      })
+      urlRequest.end()
+    })
+
     it('should throw if given an invalid session option', function (done) {
       const requestUrl = '/requestUrl'
       try {

spec/api-session-spec.js

@@ -226,7 +226,7 @@ describe('session module', function () {
     it('clears localstorage data', function (done) {
       ipcMain.on('count', function (event, count) {
         ipcMain.removeAllListeners('count')
-        assert(!count)
+        assert.equal(count, 0)
         done()
       })
       w.loadURL('file://' + path.join(fixtures, 'api', 'localstorage.html'))

spec/fixtures/api/crash.html

@@ -1,20 +1,24 @@
 <html>
 <body>
 <script type="text/javascript" charset="utf-8">
-var port = require('url').parse(window.location.href, true).query.port;
-var crashReporter = require('electron').crashReporter;
+var url = require('url').parse(window.location.href, true);
+var uploadToServer = !url.query.skipUpload;
+var port = url.query.port;
+var {crashReporter, ipcRenderer} = require('electron');
 crashReporter.start({
   productName: 'Zombies',
   companyName: 'Umbrella Corporation',
   submitURL: 'http://127.0.0.1:' + port,
-  uploadToServer: true,
+  uploadToServer: uploadToServer,
   ignoreSystemCrashHandler: true,
   extra: {
     'extra1': 'extra1',
     'extra2': 'extra2',
   }
 });
-
+if (!uploadToServer) {
+  ipcRenderer.sendSync('set-crash-directory', crashReporter.getCrashesDirectory())
+}
 setImmediate(function() { process.crash(); });
 </script>
 </body>

spec/fixtures/module/error-properties.js

@@ -0,0 +1,11 @@
+class Foo {
+  set bar (value) {
+    throw new Error('setting error')
+  }
+
+  get bar () {
+    throw new Error('getting error')
+  }
+}
+
+module.exports = new Foo()

spec/fixtures/module/preload-sandbox.js

@@ -1,6 +1,8 @@
 (function () {
+  const {setImmediate} = require('timers')
   const {ipcRenderer} = require('electron')
   window.ipcRenderer = ipcRenderer
+  window.setImmediate = setImmediate
   if (location.protocol === 'file:') {
     window.test = 'preload'
     window.require = require

spec/fixtures/no-proprietary-codecs.js

@@ -0,0 +1,47 @@
+// Verifies that Electron cannot play a video that uses proprietary codecs
+//
+// This application should be run with the ffmpeg that does not include
+// proprietary codecs to ensure Electron uses it instead of the version
+// that does include proprietary codecs.
+
+const {app, BrowserWindow, ipcMain} = require('electron')
+const path = require('path')
+const url = require('url')
+
+const MEDIA_ERR_SRC_NOT_SUPPORTED = 4
+const FIVE_MINUTES = 5 * 60 * 1000
+
+let window
+
+app.once('ready', () => {
+  window = new BrowserWindow({
+    show: false
+  })
+
+  window.loadURL(url.format({
+    protocol: 'file',
+    slashed: true,
+    pathname: path.resolve(__dirname, 'asar', 'video.asar', 'index.html')
+  }))
+
+  ipcMain.on('asar-video', (event, message, error) => {
+    if (message === 'ended') {
+      console.log('Video played, proprietary codecs are included')
+      app.exit(1)
+      return
+    }
+
+    if (message === 'error' && error === MEDIA_ERR_SRC_NOT_SUPPORTED) {
+      app.exit(0)
+      return
+    }
+
+    console.log(`Unexpected response from page: ${message} ${error}`)
+    app.exit(1)
+  })
+
+  setTimeout(() => {
+    console.log('No IPC message after 5 minutes')
+    app.exit(1)
+  }, FIVE_MINUTES)
+})

spec/webview-spec.js

@@ -121,6 +121,9 @@ describe('<webview> tag', function () {
     })
 
     it('loads node symbols after POST navigation when set', function (done) {
+      // FIXME Figure out why this is timing out on AppVeyor
+      if (process.env.APPVEYOR === 'True') return done()
+
       webview.addEventListener('console-message', function (e) {
         assert.equal(e.message, 'function object object')
         done()