663042 - Disable some breaking parts of the code in e10s builds

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

This patch disables some parts of the code in the browser window startup that would otherwise throw errors and make BrowserStartup and delayedStartup not reach the end. Also included some small fixes that were straightforward to make the permanent fix.
This, together with the patch from bug 663040, makes it possible for the web browser to come up with e10s enabled, open/close tabs and open web pages in them.

(being able to interact with the page is bug 583976)

Comment 1

[Justin Dolske [:Dolske]]

Comment on attachment 538192 [details] [diff] [review]
Patch

Review of attachment 538192 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ +1468,1 @@
>  

Hmm. I think it would be good practice to start using a consistent form that refers to a bug number, so we can track all of these. EG

#ifdef MOZ_E10S_COMPAT
  // bug 12345 - this makes the browser emit a foul smell
#else
  ...current code...
#endif

It would be nice to log a error message too, but I think most of these blocks are generic enough that it would just be console spam (ie, it's not tied to a specific user action which isn't working yet).

::: browser/base/content/tabbrowser.xml
@@ +780,4 @@
>              var docElement = this.ownerDocument.documentElement;
>              var sep = docElement.getAttribute("titlemenuseparator");
>  
> +            var docTitle = aBrowser.contentTitle;

Is is currently possible to not hit this case (.contentViewer == null)? If so, I don't think this is quite right. I guess that might happen if the document hasn't loaded in a tab yet, and I wonder if the code this ends up calling (which looks at .contentDocument.title) might throw.

Actually, what happen here, even with your change, in E10S? Seems like it should still not work.

@@ +856,5 @@
>  
>              var oldBrowser = this.mCurrentBrowser;
>              if (oldBrowser) {
>                oldBrowser.setAttribute("type", "content-targetable");
> +              oldBrowser.docShellIsActive = false;

I suppose there could, in theory, be a slight behavior change here. Currently, if docShell is |null| we'd explode and throw.

::: toolkit/content/widgets/browser.xml
@@ +262,5 @@
>                  readonly="true"/>
>  
> +      <property name="docShellIsActive"
> +                onget="return this.docShell && this.docShell.isActive;"
> +                onset="if (this.docShell) return this.docShell.isActive = val; return false;"/>

Would it make sense to have these both return |false| in E10S compat mode, via #ifdef? That would conveniently avoid the possible slight behavior change noted above.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

(In reply to comment #1)
> Hmm. I think it would be good practice to start using a consistent form that
> refers to a bug number, so we can track all of these. EG
> 
> #ifdef MOZ_E10S_COMPAT
>   // bug 12345 - this makes the browser emit a foul smell
> #else
>   ...current code...
> #endif

alright

> 
> It would be nice to log a error message too, but I think most of these
> blocks are generic enough that it would just be console spam (ie, it's not
> tied to a specific user action which isn't working yet).

yeah I don't think anything in here would benefit from a message now.

> 
> ::: browser/base/content/tabbrowser.xml
> @@ +780,4 @@
> >              var docElement = this.ownerDocument.documentElement;
> >              var sep = docElement.getAttribute("titlemenuseparator");
> >  
> > +            var docTitle = aBrowser.contentTitle;
> 
> Is is currently possible to not hit this case (.contentViewer == null)? If
> so, I don't think this is quite right. I guess that might happen if the
> document hasn't loaded in a tab yet, and I wonder if the code this ends up
> calling (which looks at .contentDocument.title) might throw.
> 
> Actually, what happen here, even with your change, in E10S? Seems like it
> should still not work.

sorry for the lack of context here. This is tied to bug 662008. In that bug, contentTitle is fixed to never access the content document directly (.contentDocument.title). Instead it has a cached value (in the chrome process) of the current page title, and when DOMTitleChanged happens in the content process, that value is updated.

So with both these changes together there will be no change in behavior here. If things haven't started up yet (and .contentViewer is null), contentTitle will conveniently be a null string instead of throwing.

> 
> @@ +856,5 @@
> >  
> >              var oldBrowser = this.mCurrentBrowser;
> >              if (oldBrowser) {
> >                oldBrowser.setAttribute("type", "content-targetable");
> > +              oldBrowser.docShellIsActive = false;
> 
> I suppose there could, in theory, be a slight behavior change here.
> Currently, if docShell is |null| we'd explode and throw.
> 
> ::: toolkit/content/widgets/browser.xml
> @@ +262,5 @@
> >                  readonly="true"/>
> >  
> > +      <property name="docShellIsActive"
> > +                onget="return this.docShell && this.docShell.isActive;"
> > +                onset="if (this.docShell) return this.docShell.isActive = val; return false;"/>
> 
> Would it make sense to have these both return |false| in E10S compat mode,
> via #ifdef? That would conveniently avoid the possible slight behavior
> change noted above.

hmm thinking about it, I don't know if it would make a difference. Seems like the docShell existence is a strong assumption in most code out there, so it must probably always be true. The only case where it's null and things would explode would then be the e10s case which is the one we're handling.
But if you had other situations in mind please let me know.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Filed bugs for the parts that were disabled

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Filed all bugs where things were disabled and adjusted the style of all the #ifdefs.

Comment 5

[Marco Bonardo [:mak]]

I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/7b3398b92495

Comment 7

[Boris Zbarsky [:bzbarsky]]

Is there a followup bug for making the docshellIsActive setter work in remote mode?  That's needed for desktop Firefox to not regress, no?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Also, there are tests that still use isActive on docshells directly....