397924 - [FIX]bug 372769 broke Tab Sidebar

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Peter van der Woude [:Peter6]]

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.

Comment 1

[Dave Townsend [:mossop]]

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);

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: 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/[e-mail]/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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: 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...

Comment 4

[Dave Townsend [:mossop]]

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?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes.  And property getters and setters.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Now explicitly defining fields that evaluate to "undefined"

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 13

[Peter van der Woude [:Peter6]]

Thanks Boris

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

VERIFIED

Comment 14

[[e-mail]]

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)