[FIX]bug 372769 broke Tab Sidebar

VERIFIED FIXED in mozilla1.9beta1

Status

()

Core
XBL
P1
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Peter6, Assigned: bz)

Tracking

({regression})

Trunk
mozilla1.9beta1
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007092803 Minefield/3.0a9pre ID:2007092803

repro:
install Tab Sidebar http://www.oxymoronical.com/web/firefox/TabSidebar
open a few tabs

result:
the previews are gone, there's just an empty sidebar.
So this is kinda broken. The first line of the xbl constructor for main tabsidebar element tries to do do something to the sidebar window. It fails with:

Error: window has no properties
Source File: chrome://tabsidebar/content/widgets/tabpreviews.xml
Line: 26

Code is:

var docshell = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
                     .getInterface(Components.interfaces.nsIWebNavigation)
                     .QueryInterface(Components.interfaces.nsIDocShellTreeItem);
Component: General → XBL
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → xbl
Hardware: PC → All
Target Milestone: --- → mozilla1.9 M9
Assignee: nobody → bzbarsky
Blocks: 372769
Priority: -- → P1
Created attachment 282805 [details] [diff] [review]
Patch for "Error: window has no properties" issue

So the thing is, this doesn't fix lack of previews for me.  I hacked install.rdf to claim compatibility with 3.0a9pre, and with this patch applied I now get:

Error: preview.init is not a function
Source File: chrome://tabsidebar/content/widgets/tabpreviews.xml
Line: 627

Of course before that I get:

XML Parsing Error: undefined entity
Location: jar:file:///Users/bzbarsky/Library/Application%20Support/Firefox/Profiles/r0zc5aue.testing/extensions/TabSidebar@blueprintit.co.uk/chrome/tabsidebar.jar!/content/widgets/tabpreview.xml
Line Number 75, Column 10:
          <xul:toolbarbutton class="tbs-close tbs-icon" anonid="close" tooltiptext="&closeTab.label;"

so it's not surprising that init() fails.  There are other entity errors in that XBL too.  Once I fix them by removing the entities, preview works fine.  I have no idea why the localization stuff is failing, but I'm willing to bet that bug 372769 is not involved.
Flags: blocking1.9?
Summary: bug 372769 broke Tab Sidebar → [FIX]bug 372769 broke Tab Sidebar
Created attachment 282807 [details] [diff] [review]
Same as diff -w

The basic issue is that I did what mrbkap suggested in bug 372769 comment 3.  That is, I defined fields that had empty bodies to JSVAL_VOID, and the same with fields that evaluated to "undefined".

This contrasts with the old code that didn't install such fields at all.

The basic issue with Tab Sidebar was code like this:

  <implementation>
    <field name="window"/>
    <constructor>
      var foo = window;  // then use it
    </constructor>
  </implementation>

Used to be that the <field/> here was just an expesive-ish no-op, so "window" resolved to the Window object.  With the patch for bug 372769 "window" resolved to this.window (since |this| is on the scope chain in the ctor), which was JSVAL_VOID, a.k.a. "undefined".

This patch basically restores the old behavior: if there is no field body or if the evaluation of the body gives "undefined", just pretend that the <field/> doesn't exist at all.  I do think that various XBL in our tree uses this pattern, and I guess extensions copied that.  We should perhaps file bugs to fix it in our tree, since it _is_ a no-op that costs CPU cycles...
Attachment #282807 - Flags: superreview?(jonas)
Attachment #282807 - Flags: review?(jonas)
Just confirming that this patch fixes the issue for me. Not sure what locale problems you were seeing but I don't seem to have a problem.

I guess I was never really aware of |this| being in scope in the constructor, is the same true of methods?
Yes.  And property getters and setters.
Comment on attachment 282807 [details] [diff] [review]
Same as diff -w

Isn't this going to cause us to reattempt to install the field over and over? Could we simply not add the field to the field-list if it doesn't have any children?

>Index: content/xbl/src/nsXBLProtoImplField.cpp
> nsXBLProtoImplField::InstallField(nsIScriptContext* aContext,
>                                   JSObject* aBoundNode,
>-                                  nsIURI* aBindingDocURI) const
>+                                  nsIURI* aBindingDocURI,
>+                                  PRBool* aDidInstall) const
> {
>   NS_PRECONDITION(aBoundNode,
>                   "uh-oh, bound node should NOT be null or bad things will "
>                   "happen");
> 
>-  jsval result = JSVAL_VOID;
>+  if (mFieldTextLength == 0) {
>+    *aDidInstall = PR_FALSE;
>+    return NS_OK;
>+  }
>+
>+  jsval result = JSVAL_NULL;

Wanna set *aDidInstall = PR_FALSE at the top of the function and only set it to true at the end? Should make it easier to make sure that it always gets set.

r/sr=me unless we can do the don't-add-field-at-all thing.
Attachment #282807 - Flags: superreview?(jonas)
Attachment #282807 - Flags: superreview+
Attachment #282807 - Flags: review?(jonas)
Attachment #282807 - Flags: review+
> Isn't this going to cause us to reattempt to install the field over and over?

Er, yes.  I need to add a boolean member to cover the case when the field evaluates to "undefined".  Otherwise it could execute multiple times, which is bad.

We could probably try dropping fields with no text in them at all, but I don't think I'll be able to try to sort through that in the near term, to be honest.  :(
Argh.  Except I need to keep track of the evaluated state on a per-field basis...

Maybe I should just give up on this, back out bug 372769 (since it didn't help Txul/Ts on tinderbox, unlike on my box), and just refix bug 372769 in a different way.  :(

The other option is to break compat for the case when we have actual text that evaluates to "undefined" and go ahead and define that value.  Jonas, what do you think?  It seems better than any setup that allows a field to evaluate multiple times....
Created attachment 283143 [details] [diff] [review]
Now explicitly defining fields that evaluate to "undefined"
Attachment #282805 - Attachment is obsolete: true
Attachment #282807 - Attachment is obsolete: true
Attachment #283143 - Flags: superreview?(jonas)
Attachment #283143 - Flags: review?(jonas)
Comment on attachment 283143 [details] [diff] [review]
Now explicitly defining fields that evaluate to "undefined"

Yeah, i'd say this is the way to go. I assume that you can't treat <field name="foo"/> as undefined because it would cause the problem in comment 3 still?

Ideally we should assert if mFieldTextLength is 0 and aBindingDocURI is a chrome-uri.

I wonder if this re-eval for empty fields thing is eating up some of the perf gain we should see here. As I really do think that the original patch should be a perf gain, no matter what tinderbox says.

r/sr/a=me
Attachment #283143 - Flags: superreview?(jonas)
Attachment #283143 - Flags: superreview+
Attachment #283143 - Flags: review?(jonas)
Attachment #283143 - Flags: review+
Attachment #283143 - Flags: approval1.9+
> because it would cause the problem in comment 3 still?

Exactly.

> Ideally we should assert if mFieldTextLength is 0 and aBindingDocURI is a
> chrome-uri.

Agreed.

> I wonder if this re-eval for empty fields thing is eating up some of the perf
> gain we should see here.

mconnor tells me we don't have any empty fields in our chrome, for what it's worth.
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: blocking1.9? → in-testsuite+
Resolution: --- → FIXED
(Reporter)

Comment 13

10 years ago
Thanks Boris

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100208 Minefield/3.0a9pre ID:2007100208

VERIFIED
Status: RESOLVED → VERIFIED

Comment 14

10 years ago
I tweaked MDC slightly, see
http://developer.mozilla.org/en/docs/index.php?title=XBL%3AXBL_1.0_Reference%3AElements&diff=58125&oldid=54865
(the second change is a whitespace changed caused by the section edit)
You need to log in before you can comment on or make changes to this bug.