Are there any errors in the browser console? Does browser.ctrlTab.previews make a difference?

(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 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 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?

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

(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?

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

(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...

(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.

(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

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

(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?

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.

(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...

(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.)

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

(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.

(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.

(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 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.

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

Verified for MacOSX 10.8.5 on 34.0a2.