Closed Bug 663042 Opened 13 years ago Closed 13 years ago

Disable some breaking parts of the code in e10s builds

Categories

(Firefox :: General, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 8

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #538192 - Flags: review?(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.
Attachment #538192 - Flags: review?(dolske) → review-
(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.
Filed bugs for the parts that were disabled
Blocks: 666816
Attached patch PatchSplinter Review
Filed all bugs where things were disabled and adjusted the style of all the #ifdefs.
Attachment #538192 - Attachment is obsolete: true
Attachment #541563 - Flags: review?(dolske)
Attachment #541563 - Flags: review?(dolske) → review+
I backed out everything from central since Android and Maemo were unhappy about the push these changes were part of.
http://hg.mozilla.org/mozilla-central/rev/7b3398b92495
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Is there a followup bug for making the docshellIsActive setter work in remote mode?  That's needed for desktop Firefox to not regress, no?
Also, there are tests that still use isActive on docshells directly....
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: