Carefully manage unhandled rejections for navigation (puppeteer#741)

Currently, navigation watcher throws exception if timeout
is exceeded.

Due to the way it is used in `page.navigate`, the promise
get's rejected before it is awaited, which is considered to
be "unhandled promise rejection".

Fixes puppeteer#738

lib/NavigatorWatcher.js

@@ -33,7 +33,7 @@ class NavigatorWatcher {
   }
 
   /**
-   * @return {!Promise<!Map<string, !Response>>}
+   * @return {!Promise<?Error>}
    */
   async waitForNavigation() {
     this._requestIds = new Set();
@@ -70,8 +70,7 @@ class NavigatorWatcher {
 
     const error = await Promise.race(navigationPromises);
     this._cleanup();
-    if (error)
-      throw new Error(error);
+    return error ? new Error(error) : null;
   }
 
   cancel() {

lib/Page.js

@@ -372,7 +372,7 @@ class Page extends EventEmitter {
     const watcher = new NavigatorWatcher(this._client, this._ignoreHTTPSErrors, options);
     const responses = new Map();
     const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Response, response => responses.set(response.url, response));
-    const result = watcher.waitForNavigation();
+    const navigationPromise = watcher.waitForNavigation();
 
     const referrer = this._networkManager.extraHTTPHeaders()['referer'];
     try {
@@ -382,8 +382,10 @@ class Page extends EventEmitter {
       watcher.cancel();
       throw e;
     }
-    await result;
+    const error = await navigationPromise;
     helper.removeEventListeners([listener]);
+    if (error)
+      throw error;
     if (this._frameManager.isMainFrameLoadingFailed())
       throw new Error('Failed to navigate: ' + url);
     return responses.get(this.mainFrame().url()) || null;
@@ -410,9 +412,10 @@ class Page extends EventEmitter {
 
     const responses = new Map();
     const listener = helper.addEventListener(this._networkManager, NetworkManager.Events.Response, response => responses.set(response.url, response));
-    await watcher.waitForNavigation();
+    const error = await watcher.waitForNavigation();
     helper.removeEventListeners([listener]);
-
+    if (error)
+      throw error;
     return responses.get(this.mainFrame().url()) || null;
   }
 

test/test.js

@@ -598,15 +598,16 @@ describe('Page', function() {
       expect(error.message).toContain('Failed to navigate');
     }));
     it('should fail when exceeding maximum navigation timeout', SX(async function() {
-      let error = null;
+      let hasUnhandledRejection = false;
+      const unhandledRejectionHandler = () => hasUnhandledRejection = true;
+      process.on('unhandledRejection', unhandledRejectionHandler);
       // Hang for request to the empty.html
       server.setRoute('/empty.html', (req, res) => { });
-      try {
-        await page.goto(PREFIX + '/empty.html', {timeout: 59});
-      } catch (e) {
-        error = e;
-      }
-      expect(error.message).toContain('Navigation Timeout Exceeded: 59ms');
+      let error = null;
+      await page.goto(PREFIX + '/empty.html', {timeout: 1}).catch(e => error = e);
+      expect(hasUnhandledRejection).toBe(false);
+      expect(error.message).toContain('Navigation Timeout Exceeded: 1ms');
+      process.removeListener('unhandledRejection', unhandledRejectionHandler);
     }));
     it('should work when navigating to valid url', SX(async function() {
       const response = await page.goto(EMPTY_PAGE);