Last Comment Bug 397924 - [FIX]bug 372769 broke Tab Sidebar
: [FIX]bug 372769 broke Tab Sidebar
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9beta1
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 372769
  Show dependency treegraph
 
Reported: 2007-09-28 10:11 PDT by Peter van der Woude [:Peter6]
Modified: 2007-10-03 01:33 PDT (History)
10 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for "Error: window has no properties" issue (9.94 KB, patch)
2007-09-28 22:57 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Same as diff -w (9.17 KB, patch)
2007-09-28 23:07 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
jonas: review+
jonas: superreview+
Details | Diff | Review
Now explicitly defining fields that evaluate to "undefined" (11.04 KB, patch)
2007-10-01 23:54 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
jonas: review+
jonas: superreview+
jonas: approval1.9+
Details | Diff | Review

Description Peter van der Woude [:Peter6] 2007-09-28 10:11:37 PDT
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] 2007-09-28 10:33:33 PDT
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 [:bz] (Out June 25-July 6) 2007-09-28 22:57:03 PDT
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.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-09-28 23:07:25 PDT
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...
Comment 4 Dave Townsend [:mossop] 2007-09-29 05:23:49 PDT
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 [:bz] (Out June 25-July 6) 2007-09-29 05:40:32 PDT
Yes.  And property getters and setters.
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2007-10-01 14:03:53 PDT
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 [:bz] (Out June 25-July 6) 2007-10-01 21:47:57 PDT
> 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 [:bz] (Out June 25-July 6) 2007-10-01 23:47:17 PDT
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 [:bz] (Out June 25-July 6) 2007-10-01 23:54:01 PDT
Created attachment 283143 [details] [diff] [review]
Now explicitly defining fields that evaluate to "undefined"
Comment 10 Jonas Sicking (:sicking) PTO Until July 5th 2007-10-02 00:11:43 PDT
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 [:bz] (Out June 25-July 6) 2007-10-02 00:14:03 PDT
> 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 [:bz] (Out June 25-July 6) 2007-10-02 09:55:36 PDT
Checked in.
Comment 13 Peter van der Woude [:Peter6] 2007-10-02 09:58:50 PDT
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 neil@parkwaycc.co.uk 2007-10-03 01:33:21 PDT
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)

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