1055507 - Ctrl+Tab doesn't switch tabs in e10s mode

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Comment 1

[Dão Gottwald [:dao]]

Are there any errors in the browser console?

Does browser.ctrlTab.previews make a difference?

Comment 2

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #1)
> Are there any errors in the browser console?

Yes:

TypeError: Argument 1 of CanvasRenderingContext2D.drawWindow does not implement interface Window. browser.js:6743

> Does browser.ctrlTab.previews make a difference?

That is actually what triggers the bug.  :-)  We're trying to pass a window from the content process to drawWindow in the chrome process.  I'll see if I can fix this.

Comment 3

[(no longer active)]

Attachment: Eat the exception thrown from drawWindow in e10s mode when using tab previews; r=dao

Comment 4

[(no longer active)]

Comment on attachment 8475298 [details] [diff] [review]
Eat the exception thrown from drawWindow in e10s mode when using tab previews; r=dao

This fixes tab switching for me, but of course gives black screenshots in the tab preview dialog.  I think that's fine for now since this is not a pref that is enabled by default.  I can file a follow-up bug for fixing the screenshots.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8475298 [details] [diff] [review]
Eat the exception thrown from drawWindow in e10s mode when using tab previews; r=dao

I'm not the hugest fan of silently eating exceptions. Is it worth putting something in the console with Cu.reportError here? Or perhaps making sure we only eat these exceptions if gMultiProcessBrowser is true?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yeah, I think we should just skip over the tab preview code entirely in e10s.

Comment 7

[(no longer active)]

(In reply to Mike Conley (:mconley) from comment #5)
> Comment on attachment 8475298 [details] [diff] [review]
> Eat the exception thrown from drawWindow in e10s mode when using tab
> previews; r=dao
> 
> I'm not the hugest fan of silently eating exceptions. Is it worth putting
> something in the console with Cu.reportError here? Or perhaps making sure we
> only eat these exceptions if gMultiProcessBrowser is true?

You mean rethrow |e| if (!gMultiProcessBrowser)?  Can I just access |gMultiProcessBrowser| from this code?

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Yes, gMultiProcessBrowser will be accessible from here. Maybe just have capture return early if gMultiProcessBrowser is true.

Comment 9

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #6)
> Yeah, I think we should just skip over the tab preview code entirely in e10s.

I looked into doing that briefly, but the issue is that the tab switching code (living in ctrlTab.advanceFocus for example) is pretty intertwined with the tab preview code, and I couldn't find an easy way to disentangle them without basically rewriting the entire component...

Comment 10

[(no longer active)]

(In reply to Mike Conley (:mconley) from comment #8)
> Yes, gMultiProcessBrowser will be accessible from here. Maybe just have
> capture return early if gMultiProcessBrowser is true.

Return what though?  Its caller (tabPreviews.get) expects capture to return a DOM element.  Returning an "empty" canvas is effectively what my patch does now.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> (In reply to Mike Conley (:mconley) from comment #8)
> > Yes, gMultiProcessBrowser will be accessible from here. Maybe just have
> > capture return early if gMultiProcessBrowser is true.
> 
> Return what though?  Its caller (tabPreviews.get) expects capture to return
> a DOM element.  Returning an "empty" canvas is effectively what my patch
> does now.

Right, but it also eats and exceptions that might get raised in drawWindow for non-e10s windows.

Maybe the best approach would be just to wrap all of that canvas[1] stuff in a conditional checking gMultiProcessBrowser, and just skip over it if true.

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-tabPreviews.js?from=browser-tabPreviews.js#61-67

Comment 12

[Dão Gottwald [:dao]]

This is basically bug 698371. And no, I don't think we should just eat the exception.

Comment 13

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #12)
> This is basically bug 698371. And no, I don't think we should just eat the
> exception.

OK, so do you agree with comment 11?

Comment 14

[Dão Gottwald [:dao]]

Ideally we'd just fix bug 698371, which sooner or later needs to happen anyway. Whether we should add a workaround here in the meantime depends on whether we consider this essential for getting e10s tested by nightly users. Since it's behind a hidden pref, I'm not convinced of this.

Comment 15

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #14)
> Ideally we'd just fix bug 698371, which sooner or later needs to happen
> anyway. Whether we should add a workaround here in the meantime depends on
> whether we consider this essential for getting e10s tested by nightly users.
> Since it's behind a hidden pref, I'm not convinced of this.

Well it gets in the way of my dogfooding, which is why I filed this.  As things stand currently, I can only tab switch using the mouse, or through Cmd+Alt+Left/Right, both of which will be things that will cause me to stop testing e10s in my main browser when I get tired of tab switching not working...

Comment 16

[(no longer active)]

(Note that we have the option of backing out the patch here when bug 698371 gets fixed.  I'm not suggesting that we should *ship* the code that disables screenshotting like this.)

Comment 17

[Dão Gottwald [:dao]]

I'm not opposed to adding a gMultiProcessBrowser check. Just make sure you also add a comment pointing to bug 698371.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #15)
> Well it gets in the way of my dogfooding, which is why I filed this.  As
> things stand currently, I can only tab switch using the mouse, or through
> Cmd+Alt+Left/Right, both of which will be things that will cause me to stop
> testing e10s in my main browser when I get tired of tab switching not
> working...

Can you just turn off the pref? That should make ctrl-tab work fine, but without thumbnails, which seems similar to what you would get with this patch.

Comment 19

[Dão Gottwald [:dao]]

(In reply to Bill McCloskey (:billm) from comment #18)
> Can you just turn off the pref? That should make ctrl-tab work fine, but
> without thumbnails, which seems similar to what you would get with this
> patch.

No, the UI with the thumbnails also lets you switch tabs in most recently used order.

Comment 20

[(no longer active)]

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to Bill McCloskey (:billm) from comment #18)
> > Can you just turn off the pref? That should make ctrl-tab work fine, but
> > without thumbnails, which seems similar to what you would get with this
> > patch.
> 
> No, the UI with the thumbnails also lets you switch tabs in most recently
> used order.

Yes, which is the reason why I have that pref turned on.  I can already get the usual Ctrl+Tab behavior by Cmd+Alt+Left/Right, but The Right Way to switch tabs is MRU.  ;-)

Comment 21

[(no longer active)]

Attachment: Avoid calling drawWindow in e10s mode when using tab previews until bug 698371 gets fixed; r=dao

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8476055 [details] [diff] [review]
Avoid calling drawWindow in e10s mode when using tab previews until bug 698371 gets fixed; r=dao

>-    var ctx = thumbnail.getContext("2d");
>-    var win = aTab.linkedBrowser.contentWindow;
>-    var snippetWidth = win.innerWidth * .6;
>-    var scale = this.width / snippetWidth;
>-    ctx.scale(scale, scale);
>-    ctx.drawWindow(win, win.scrollX, win.scrollY,
>-                   snippetWidth, snippetWidth * this.aspectRatio, "rgb(255,255,255)");
>+    // This check should be removed once bug 698371 gets fixed.
>+    if (!gMultiProcessBrowser) {
>+      var ctx = thumbnail.getContext("2d");
>+      var win = aTab.linkedBrowser.contentWindow;
>+      var snippetWidth = win.innerWidth * .6;
>+      var scale = this.width / snippetWidth;
>+      ctx.scale(scale, scale);
>+      ctx.drawWindow(win, win.scrollX, win.scrollY,
>+                     snippetWidth, snippetWidth * this.aspectRatio, "rgb(255,255,255)");
>+    }

A more lightweight way to do this without reindenting existing code:

    // drawWindow doesn't yet work with e10s (bug 698371)
    if (gMultiProcessBrowser)
      return thumbnail;

    var ctx = thumbnail.getContext("2d");
    ...

The only difference is that we would never set aTab.__thumbnail, but that should be fine as that cache isn't useful anyway without drawWindow.

Comment 23

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e024c7a1fef9

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e024c7a1fef9

Comment 25

[bakolden5]

Verified: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Verified for MacOSX 10.8.5 on 34.0a2.