Last Comment Bug 663042 - Disable some breaking parts of the code in e10s builds
: Disable some breaking parts of the code in e10s builds
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 All
: -- normal with 1 vote (vote)
: Firefox 8
Assigned To: :Felipe Gomes (needinfo me!)
:
Mentors:
Depends on: 662390
Blocks: 666801 666804 666809 666816 1096804
  Show dependency treegraph
 
Reported: 2011-06-08 23:43 PDT by :Felipe Gomes (needinfo me!)
Modified: 2016-01-13 12:16 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.93 KB, patch)
2011-06-08 23:43 PDT, :Felipe Gomes (needinfo me!)
dolske: review-
Details | Diff | Review
Patch (11.65 KB, patch)
2011-06-23 17:47 PDT, :Felipe Gomes (needinfo me!)
dolske: review+
Details | Diff | Review

Description :Felipe Gomes (needinfo me!) 2011-06-08 23:43:40 PDT
Created attachment 538192 [details] [diff] [review]
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] 2011-06-20 16:19:52 PDT
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 me!) 2011-06-21 16:38:46 PDT
(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 me!) 2011-06-23 17:25:42 PDT
Filed bugs for the parts that were disabled
Comment 4 :Felipe Gomes (needinfo me!) 2011-06-23 17:47:55 PDT
Created attachment 541563 [details] [diff] [review]
Patch

Filed all bugs where things were disabled and adjusted the style of all the #ifdefs.
Comment 5 Marco Bonardo [::mak] 2011-07-01 07:48:02 PDT
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 me!) 2011-07-08 13:21:05 PDT
http://hg.mozilla.org/mozilla-central/rev/7b3398b92495
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-11 06:14:42 PDT
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 [:bz] (Out June 25-July 6) 2011-07-11 06:19:36 PDT
Also, there are tests that still use isActive on docshells directly....

Note You need to log in before you can comment on or make changes to this bug.