Last Comment Bug 94199 - fastload XBL methods and properties
: fastload XBL methods and properties
Status: RESOLVED FIXED
[ts], mobilestartupshrink
: perf
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: mozilla10
Assigned To: Neil Deakin
:
Mentors:
: 529170 (view as bug list)
Depends on: 684700 696724 70647 112064 520309 700172 701273 701662 723676 CVE-2012-0452 732495 773606
Blocks: 447581 506416 697456 737607 7251 49141 506431 697449
  Show dependency treegraph
 
Reported: 2001-08-07 15:03 PDT by Cathleen
Modified: 2014-04-25 15:15 PDT (History)
62 users (show)
benjamin: blocking1.8a4-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-


Attachments
instrumentation for collecting time spending on load XBLs (3.17 KB, patch)
2006-05-23 16:56 PDT, Feng Qian (Google)
no flags Details | Diff | Splinter Review
Part 1: move handling of base binding to nsXBLPrototypeBinding (17.44 KB, patch)
2011-07-17 16:24 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 2: add method to serialize/deserialize js function (8.86 KB, patch)
2011-07-17 16:25 PDT, Neil Deakin
mwu.code: review+
Details | Diff | Splinter Review
Part 3 - add GetValue method to nsISupportsKey (609 bytes, patch)
2011-07-17 16:28 PDT, Neil Deakin
benjamin: review+
Details | Diff | Splinter Review
Part 4 - serialize/deserialize bindings to startup cache, work in progress (74.11 KB, patch)
2011-07-17 16:29 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 2.1: add method to serialize/deserialize js function (8.88 KB, patch)
2011-07-19 07:43 PDT, Neil Deakin
enndeakin: review+
Details | Diff | Splinter Review
Part 1.2: move handling of base binding to nsXBLPrototypeBinding (17.43 KB, patch)
2011-09-09 11:56 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 4.2 - serialize/deserialize bindings to startup cache (81.00 KB, patch)
2011-09-09 12:05 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 1.3: move handling of base binding to nsXBLPrototypeBinding (17.57 KB, patch)
2011-09-30 06:31 PDT, Neil Deakin
bzbarsky: review+
Details | Diff | Splinter Review
Part 4.3 - serialize/deserialize bindings to startup cache (81.23 KB, patch)
2011-09-30 06:32 PDT, Neil Deakin
bzbarsky: review-
Details | Diff | Splinter Review
mozmill automated test (13.74 KB, patch)
2011-09-30 06:34 PDT, Neil Deakin
hskupin: feedback-
Details | Diff | Splinter Review
Part 1.4: move handling of base binding to nsXBLPrototypeBinding (21.75 KB, patch)
2011-10-19 07:21 PDT, Neil Deakin
enndeakin: review+
Details | Diff | Splinter Review
Mozmill automated test, version 2 (15.00 KB, patch)
2011-10-19 07:24 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 4.4 - serialize/deserialize bindings to startup cache (98.04 KB, patch)
2011-10-31 18:08 PDT, Neil Deakin
bzbarsky: review+
Details | Diff | Splinter Review
Part 5.1 - serialize/deserialize property and method line numbers (17.56 KB, patch)
2011-10-31 18:10 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Patch differences (95.83 KB, patch)
2011-10-31 18:17 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Part 5.2 - just remove line number handling (6.83 KB, patch)
2011-11-02 17:14 PDT, Neil Deakin
bzbarsky: review+
Details | Diff | Splinter Review
Part 6: Cleanup (6.10 KB, patch)
2011-11-06 13:59 PST, :Ms2ger
enndeakin: review+
Details | Diff | Splinter Review

Description Cathleen 2001-08-07 15:03:23 PDT
to help startup performance, hyatt is planning on making XBL fastload
Comment 1 Brendan Eich [:brendan] 2001-08-07 16:31:32 PDT
I think this depends on XBL Brutal Sharing, which I believe mscott has working
pretty well now -- mscott, how's it going?  Should we note the bug dependency?

/be
Comment 2 sairuh (rarely reading bugmail) 2001-08-07 19:33:14 PDT
afaik, either jrgm or paw would be the best qa contacts for this one. :)
Comment 3 Peter Trudelle 2001-12-01 13:09:13 PST
Will this really land by Dec. 11?  Is it needed for mozilla1.0?  MachV?  Will it
affect Gecko?
Comment 4 Cathleen 2001-12-03 20:16:38 PST
fastload XBL and CSS will help on startup, machV and moz1.0 not for embed
Comment 5 Jaime Rodriguez, Jr. 2002-01-11 09:31:25 PST
what are the chances this will make 098?
Comment 6 Jan Varga [:janv] 2003-03-11 03:23:12 PST
taking for now
Comment 7 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2004-09-15 10:54:40 PDT
worcester12345: a feature bug without a patch is not even remotely a candidate
for a blocking flag
Comment 8 Daniel Veditz [:dveditz] 2004-09-15 18:50:09 PDT
Blocking nominations can be made of any bug, that's how we track unfinished
features scheduled to land. It's the "approval" flags that make no sense without
a reviewed patch. 

Blocking-minus means turned down by drivers, so that's not right either. But
realistically a bug that we've lived with for three years is never going to be
approved as a blocker so I'll leave the minus.
Comment 9 Feng Qian (Google) 2006-05-23 16:56:23 PDT
Created attachment 223115 [details] [diff] [review]
instrumentation for collecting time spending on load XBLs

The patch instruments nsXBLService to meausre the time spent on loading XBL files. Please correct me if I instrumented wrong place. Start the browser and shut it down, about 3% ~ 7% were spent on this loading XBLs (on Linux, with other workloads).
Comment 10 Feng Qian (Google) 2006-05-23 17:00:20 PDT
(In reply to comment #9)
> Created an attachment (id=223115) [edit]
> instrumentation for collecting time spending on load XBLs
> 
> The patch instruments nsXBLService to meausre the time spent on loading XBL
> files. Please correct me if I instrumented wrong place. Start the browser and
> shut it down, about 3% ~ 7% were spent on this loading XBLs (on Linux, with
> other workloads).

The number is inaccurate because the profile directory resides on NFS. 


Comment 11 Antoine Azar [:antoine] 2008-11-18 09:13:26 PST
I'd like to revive this 7 year old bug. Profiling on mobile N810 shows that XBL binding and installing takes a huge chunk of startup time. We'd likely save 3 seconds if we could have this made persistent.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2008-11-18 10:31:24 PST
It'd rock if you could do this! Assigning to you.
Comment 13 Brendan Eich [:brendan] 2008-11-18 12:49:43 PST
I will help -- ask questions here or by mail. Thanks,

/be
Comment 14 Antoine Azar [:antoine] 2008-12-03 12:35:07 PST
I think Taras is going to have a look at it first.
Comment 15 (dormant account) 2011-03-17 09:31:48 PDT
*** Bug 529170 has been marked as a duplicate of this bug. ***
Comment 16 Dietrich Ayala (:dietrich) 2011-04-13 10:02:44 PDT
Comment #1 in duped bug 529170 talks to how to do this in the contemporary world of startupcache.
Comment 17 Neil Deakin 2011-07-17 16:24:21 PDT
Created attachment 546448 [details] [diff] [review]
Part 1: move handling of base binding to nsXBLPrototypeBinding
Comment 18 Neil Deakin 2011-07-17 16:25:48 PDT
Created attachment 546449 [details] [diff] [review]
Part 2: add method to serialize/deserialize js function
Comment 19 Neil Deakin 2011-07-17 16:28:05 PDT
Created attachment 546450 [details] [diff] [review]
Part 3 - add GetValue method to nsISupportsKey
Comment 20 Neil Deakin 2011-07-17 16:29:03 PDT
Created attachment 546451 [details] [diff] [review]
Part 4 - serialize/deserialize bindings to startup cache, work in progress
Comment 21 Michael Wu [:mwu] 2011-07-18 22:42:58 PDT
Comment on attachment 546449 [details] [diff] [review]
Part 2: add method to serialize/deserialize js function

Review of attachment 546449 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly nits, except the comments on the handling the line number. I'm not sure if I'm allowed to review the jsapi parts (or anything in xbl for that matter), but they do look straightforward and correct. I'd suggest also getting a review from Igor or some other js person for the js part.

::: content/xbl/src/nsXBLSerialize.cpp
@@ +15,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + *   Mozilla Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2010

2011

@@ +84,5 @@
> +                        void **aScriptObject)
> +{
> +  *aScriptObject = nsnull;
> +
> +  nsresult rv = aStream->Read32(aLineNumber);

check rv.

Also, I assume that the code that calls XBL_SerializeFunction wrote the line number. It might make more sense to have XBL_SerializeFunction write the line number for consistency.

@@ +90,5 @@
> +  JSObject* functionObject = nsnull;
> +
> +  PRUint32 size;
> +  rv = aStream->Read32(&size);
> +  if (NS_FAILED(rv)) return rv;

return rv on next line.

@@ +94,5 @@
> +  if (NS_FAILED(rv)) return rv;
> +
> +  char* data;
> +  rv = aStream->ReadBytes(size, &data);
> +  if (NS_FAILED(rv)) return rv;

ditto

@@ +112,5 @@
> +
> +    uint32 junk;
> +    data = (char*) ::JS_XDRMemGetData(xdr, &junk);
> +    if (data)
> +        ::JS_XDRMemSetData(xdr, NULL, 0);

Is the check here necessary? Can we just clear all the time?

@@ +116,5 @@
> +        ::JS_XDRMemSetData(xdr, NULL, 0);
> +    ::JS_XDRDestroy(xdr);
> +  }
> +
> +  if (data)

no null check necessary

::: content/xbl/src/nsXBLSerialize.h
@@ +15,5 @@
> + * The Original Code is mozilla.org code.
> + *
> + * The Initial Developer of the Original Code is
> + *   Mozilla Corporation.
> + * Portions created by the Initial Developer are Copyright (C) 2010

2011

@@ +38,5 @@
> +#ifndef nsXBLSerialize_h__
> +#define nsXBLSerialize_h__
> +
> +#include "jsapi.h"
> +#include "nsIScriptContext.h"

class nsIScriptContext;

@@ +39,5 @@
> +#define nsXBLSerialize_h__
> +
> +#include "jsapi.h"
> +#include "nsIScriptContext.h"
> +#include "nsIObjectInputStream.h"

class nsIObjectInputStream;

@@ +40,5 @@
> +
> +#include "jsapi.h"
> +#include "nsIScriptContext.h"
> +#include "nsIObjectInputStream.h"
> +#include "nsIObjectOutputStream.h"

class nsIObjectOutputStream;

::: js/src/jsxdrapi.cpp
@@ +684,5 @@
> +JS_XDRFunctionObject(JSXDRState *xdr, JSObject **objp)
> +{
> +    XDRScriptState fstate(xdr);
> +
> +    if (xdr->mode == JSXDR_ENCODE) {

wrong indentation in block
Comment 22 Neil Deakin 2011-07-19 07:41:36 PDT
Comment on attachment 546449 [details] [diff] [review]
Part 2: add method to serialize/deserialize js function

Additional review as requested.
Comment 23 Neil Deakin 2011-07-19 07:43:10 PDT
Created attachment 546782 [details] [diff] [review]
Part 2.1: add method to serialize/deserialize js function

Review of updated patch instead.
Comment 24 Igor Bukanov 2011-07-19 12:58:27 PDT
(In reply to comment #23)
> Created attachment 546782 [details] [diff] [review] [review]
> Part 2.1: add method to serialize/deserialize js function

The patch exposes js_XSRFunctionObject. But that is an internal function that is used to serialize only functions embedded in a script. It is not supposed to be used with an arbitrary function object.
Comment 25 Neil Deakin 2011-07-19 13:20:56 PDT
(In reply to comment #24)
> The patch exposes js_XSRFunctionObject. But that is an internal function
> that is used to serialize only functions embedded in a script. It is not
> supposed to be used with an arbitrary function object.

So what is the right way to serialize a function?
Comment 26 Igor Bukanov 2011-07-19 14:13:05 PDT
(In reply to comment #25)
> So what is the right way to serialize a function?

It is not clear what kind of functions would be serialized. Can we have an example how this should work and what kind of JS will be serialized?
Comment 27 Boris Zbarsky [:bz] 2011-07-19 14:16:25 PDT
The way XBL works is that it compiles some functions with JS_CompileFunction.  Then it reuses them over and over by cloning them into different scopes, where they're attached as properties to some objects so they can be called.

So what XBL has is a scripted function object (if that was your concern), but no actual script text containing the "function" keyword.
Comment 28 Igor Bukanov 2011-07-19 14:55:00 PDT
(In reply to comment #27)
> So what XBL has is a scripted function object (if that was your concern),
> but no actual script text containing the "function" keyword.

Then XDRFunctionObject may work, but I am not sure that the implementation is fully compatible with a function returned by JS_CompileFunction. I will write about it tomorrow.
Comment 29 Igor Bukanov 2011-07-20 05:28:56 PDT
Comment on attachment 546782 [details] [diff] [review]
Part 2.1: add method to serialize/deserialize js function

Review of attachment 546782 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsxdrapi.cpp
@@ +680,5 @@
>          xdr->cx->free_((void *)filename);
>  }
>  
>  JS_PUBLIC_API(JSBool)
> +JS_XDRFunctionObject(JSXDRState *xdr, JSObject **objp)

This should be named JS_XDRCompiledFunction.

@@ +687,5 @@
> +
> +    if (xdr->mode == JSXDR_ENCODE) {
> +        JSFunction* fun = GET_FUNCTION_PRIVATE(cx, *objp);
> +        if (!fun)
> +            return false;

Replace this 3 lines with the following lines that asserts that we serialize indeed the result of JS_CompileFunction:

/*
 * Check that we have unclonned result of JS_Compile*Function*
 */
JS_ASSERT(GET_FUNCTION_PRIVATE(cx, *objp) == *objp);
JSFunction* fun = static_cast<JSFunction *>(*onjp);

::: js/src/jsxdrapi.h
@@ +185,5 @@
>  extern JS_PUBLIC_API(JSBool)
>  JS_XDRValue(JSXDRState *xdr, jsval *vp);
>  
>  extern JS_PUBLIC_API(JSBool)
> +JS_XDRFunctionObject(JSXDRState *xdr, JSObject **objp);

Add comments that this should be used only against the result of JS_CompileFunction and that the deserialized function has null proto and parent.
Comment 30 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-22 09:13:17 PDT
I unbit-rotted the patches and tested on Android (Nexus One) and saw a ~10% startup improvement using the "main", "sessionrestored" and "firstpaint" metrics.

The same builds on a Samsung Galaxy Tab showed slightly worse (<1%) to no improvements.
Comment 31 Benjamin Stover (:stechz) 2011-07-29 09:50:17 PDT
What's the status on getting this landed? Let's move this forward.
Comment 32 Neil Deakin 2011-09-09 11:56:03 PDT
Created attachment 559530 [details] [diff] [review]
Part 1.2: move handling of base binding to nsXBLPrototypeBinding
Comment 33 Neil Deakin 2011-09-09 12:05:01 PDT
Created attachment 559534 [details] [diff] [review]
Part 4.2 - serialize/deserialize bindings to startup cache

This patch is more polished and now works without leaking. Unfortunately, the testsuites don't exercise the reading code much, as it requires restarting the browser, and running without any cache invalidation occurring, but I''m going to investigate some means of testing it. 

Who'd like to review?
Comment 34 Neil Deakin 2011-09-30 06:31:04 PDT
Created attachment 563724 [details] [diff] [review]
Part 1.3: move handling of base binding to nsXBLPrototypeBinding
Comment 35 Neil Deakin 2011-09-30 06:32:11 PDT
Created attachment 563725 [details] [diff] [review]
Part 4.3 - serialize/deserialize bindings to startup cache

Fixed an issue where namespace prefixes were not being saved.
Comment 36 Neil Deakin 2011-09-30 06:34:25 PDT
Created attachment 563727 [details] [diff] [review]
mozmill automated test

This test starts the browser and serializes the content tree, including anonymous nodes to a file. Upon restarting, xbl is read from the cache and the content tree is compared to the original file.
Comment 37 Boris Zbarsky [:bz] 2011-10-03 09:10:17 PDT
Comment on attachment 563724 [details] [diff] [review]
Part 1.3: move handling of base binding to nsXBLPrototypeBinding

This changeset needs a commit comment.

You need to actually remove the mHasBaseProto member.

mBaseBindingURI doesn't have anything to do with "the cache"; it's just the URI of our base binding, right?

The getter should be called GetBaseBindingURI, since GetBaseURI means something pretty specific usually and is NOT what we want here.

>+++ b/content/xbl/src/nsXBLService.cpp
>+  nsIURI* baseURI;

baseBindingURI, please.

>+      NS_ENSURE_SUCCESS(rv, rv); // Binding not yet ready or an error occurred.

I don't think rv can possibly be failure there; that line should just go away.

>-            NS_ASSERTION(!IsChromeOrResourceURI(aURI),
>-                         "Invalid extends value");

This assertion got lost.

r=me with those issues fixed.
Comment 38 Henrik Skupin (:whimboo) 2011-10-06 07:35:25 PDT
Comment on attachment 563727 [details] [diff] [review]
mozmill automated test

Patch looks good for your first test for Mozmill. I have some comments, which should be addressed or discussed first. It's a new type of test, and we have to figure out how to handle that correctly - more inline.

>+++ b/lib/elementtree.js

Could the functions become part of the dom-utils module? Seems like it's kinda related to handling of DOM nodes and exporting them.

>+function serializeDocumentStructure(node, filter, filename)

When using Document in the function name I would assume it will only work for whole documents. But as I read it would also work for each node in the DOM tree. Could we name it like serializeTree or somewhat similar to make it better understandable?

>+  var output = serializeDocumentStructureInner(node, filter, "");
>+  if (filename)
>+    outputToFile(filename, output);
>+  return output;

I would simply return the serialized string and call outputToFile from within the test itself.

>+  if (node.nodeValue)

We try to avoid strict warnings. Please use 'in' to check if the property nodeValue exists. It also applies to other if conditions in this function.

>+    for (var a = 0; a < attrslist.length; a++)
>+      attrs.push(attrslist[a]);

Can you please use i as counter. If we use a counter it's our standard across modules. Otherwise use forEach() which is our preferred method here.

>+      if (node.namespaceURI)
>+        output += node.getAttributeNS(attr.namespaceURI, attr.localName);

I assume you wanted to check for attr.namespaceURI here and not node.namespaceURI.

>+function outputToFile(filename, output)

It should better be in a new module for handling files. I would prefer saveToFile().

>+++ b/tests/functional/restartTests/testStartupXBLCache/test1.js

While this will work for your testing purposes, we would have to discuss first which test-run should hold this test. We can't put it into the functional one because it maps 1-1 with our Litmus tests. Therefore we would have to create a new one. IMO it should be related to developer tests which cannot be done by any other test framework.

>+var tabs = require("../../../../lib/tabs");

You don't need this module. Also applies to test3.js.

>+var setupModule = function() {
[..]
>+  var node = controller.window.document.documentElement;
>+  elementtree.serializeDocumentStructure(node, filterNodes, "xblcache-pre-output.txt");

I wonder if you want to have this as part of setupModule or a real test function.

>+function filterNodes(node)
>+{
>+  var name = node.localName;
>+  if (name == "notificationbox" || name == "toolbarspring") {
>+    node.removeAttribute("id");
>+  }
>+  else if (name == "tab") {
>+    node.removeAttribute("linkedpanel");
>+  }

Can you please add a comment what this filter is used for in this test? That would help a lot to understand why we are removing attributes.

>+++ b/tests/functional/restartTests/testStartupXBLCache/test2.js

Why is that test empty? Would be helpful to see at least a description.

>+++ b/tests/functional/restartTests/testStartupXBLCache/test3.js
[..]
>+  var postOutput = elementtree.serializeDocumentStructure(node, filterNodes);
>+
>+  var preOutput = readFile("xblcache-pre-output.txt");

So it seems like that the file is only used to transfer the content between tests. We have more elegant ways of doing that with i.e. the persisted object. How large will such a serialized document become? I assume it's larger than 64kb?

>+  expect.equal(preOutput, postOutput, "Original non-cached content does not match read-in cached content");

The message should have a positive wording, means what you expect here.

>+// The filter is used to skip some things which are random or will likely vary 
>+function filterNodes(node)
>+{
>+  var name = node.localName;
>+  if (name == "notificationbox" || name == "toolbarspring") {
>+    node.removeAttribute("id");
>+  }
>+  else if (name == "tab") {
>+    node.removeAttribute("linkedpanel");
>+  }
>+  else if (name == "label" && node.id == "sidebar-title") {
>+    node.removeAttribute("value"); // the value exists as it would have been persisted
>+  }

You don't strip the value attribute from the sidebar-title node in the first test. Is that expected?

>+function readFile(filename)

Same as mentioned above. I would prefer a new module for file handling and a name like readFromFile().
Comment 39 Neil Deakin 2011-10-06 07:45:56 PDT
> >+  if (node.nodeValue)
> 
> We try to avoid strict warnings. Please use 'in' to check if the property
> nodeValue exists. It also applies to other if conditions in this function.

This isn't checking for existence of nodeValue, it's checking for a non-null nodeValue.


> While this will work for your testing purposes, we would have to discuss
> first which test-run should hold this test. We can't put it into the
> functional one because it maps 1-1 with our Litmus tests. Therefore we would
> have to create a new one. IMO it should be related to developer tests which
> cannot be done by any other test framework.

Could you give a suggestion as to where to put this test?


> >+++ b/tests/functional/restartTests/testStartupXBLCache/test2.js
> 
> Why is that test empty? Would be helpful to see at least a description.

There is a comment in the test2.js Is it not clear enough?


> So it seems like that the file is only used to transfer the content between
> tests. We have more elegant ways of doing that with i.e. the persisted
> object. How large will such a serialized document become? I assume it's
> larger than 64kb?

The output can be several megabytes.


> You don't strip the value attribute from the sidebar-title node in the first
> test. Is that expected?

I could use the same function, but some specific changes are only relevant to one test. In this case, a persisted value that doesn't exist during the first test as it will be saved after the first run.
Comment 40 Henrik Skupin (:whimboo) 2011-10-06 08:02:41 PDT
(In reply to Neil Deakin from comment #39)
> > While this will work for your testing purposes, we would have to discuss
> > first which test-run should hold this test. We can't put it into the
> > functional one because it maps 1-1 with our Litmus tests. Therefore we would
> > have to create a new one. IMO it should be related to developer tests which
> > cannot be done by any other test framework.
> 
> Could you give a suggestion as to where to put this test?

Please start a discussion on our automation mailing list or dev.automation newsgroup.

> > Why is that test empty? Would be helpful to see at least a description.
> 
> There is a comment in the test2.js Is it not clear enough?

Missed that. Sorry. No, it's good enough.

> The output can be several megabytes.

Ok, so we have to use files for now. But please remove it in the teardownModule() function of test3.js. Could you even store the file under tmp? By using persisted.fileName in the test method you can transfer the filename between different tests.

> I could use the same function, but some specific changes are only relevant
> to one test. In this case, a persisted value that doesn't exist during the
> first test as it will be saved after the first run.

Can you please add this as comment? It will lower the amount of questions.
Comment 41 Henrik Skupin (:whimboo) 2011-10-07 00:07:09 PDT
(In reply to Henrik Skupin (:whimboo) from comment #40)
> > Could you give a suggestion as to where to put this test?
> 
> Please start a discussion on our automation mailing list or dev.automation
> newsgroup.

For now please just use the location you currently have. We will investigate the final test-run in parallel to your work, and I will move the test later once it has been decided where it should finally live.
Comment 42 Neil Deakin 2011-10-19 07:21:49 PDT
Created attachment 568040 [details] [diff] [review]
Part 1.4: move handling of base binding to nsXBLPrototypeBinding

Updated patch
Comment 43 Neil Deakin 2011-10-19 07:24:52 PDT
Created attachment 568042 [details] [diff] [review]
Mozmill automated test, version 2
Comment 44 Henrik Skupin (:whimboo) 2011-10-24 04:14:37 PDT
Comment on attachment 568042 [details] [diff] [review]
Mozmill automated test, version 2

Please lets move over the work on this patch to bug 696724, which will be our own product/group for Mozmill tests.
Comment 45 Boris Zbarsky [:bz] 2011-10-25 15:14:25 PDT
Comment on attachment 563725 [details] [diff] [review]
Part 4.3 - serialize/deserialize bindings to startup cache

Meta-comment: there are various places here that run way over the 80 char line.  Not sure whether you meant to do that.

Another meta-comment: when you address the review comments, I'd love to see an interdiff in addition to the new patch version.

>+++ b/content/xbl/src/nsXBLDocumentInfo.cpp
>+nsXBLDocumentInfo::RemovePrototypeBinding(const nsACString& aRef)
>+    const nsPromiseFlatCString& flat = PromiseFlatCString(aRef);
>+    nsCStringKey key(flat.get());

Man.  nsCStringKey is a broken little puppy...

I don't think you need the .get() there.  You do need a comment explaining that you need the flat string to avoid it making its own copy of the string, because that's completely non-obvious from this code.

>+EnumerateBindings(nsHashKey *aKey, void *aData, void* aClosure)
>+{
>+  nsXBLPrototypeBinding* binding = (nsXBLPrototypeBinding*)aData;
>+  binding->Write((nsIObjectOutputStream*)aClosure);

I'd prefer static_cast here.

Also, call this function WriteBinding, please.

>+nsXBLDocumentInfo::ReadPrototypeBindings(nsIURI* aURI, nsXBLDocumentInfo** aDocInfo)
>+  nsAutoArrayPtr<char> buf;

Can you please file a followup bug to fix the startup cache here?  Sometimes it returns buffers from GetBuffer allocated via malloc, sometimes ones allocated via new[] (in fact, nsZipItemPtr is already inconsistent).  There is no API documentation for how callers should be freeing the buffers.

I'm somewhat surprised it manages to not crash, actually; using delete[] on memory allocated with malloc is usually not a recipe for happiness.  And then this is passed to an object input stream, which will free via NS_Free.... so at that point we might be calling NS_Free on memory allocated with new[].  This is all completely broken.  :(

>+  nsCOMPtr<nsIPrincipal> principal;
>+  nsContentUtils::GetSecurityManager()->GetSystemPrincipal(getter_AddRefs(principal));

There was no check for a system principal when writing bindings out.  There should be one.  Chrome/resource URIs may not have system principals, last I checked.

>+  nsContentUtils::CreateDocument(NS_LITERAL_STRING("http://www.mozilla.org/xbl"),
>+                                 NS_LITERAL_STRING("bindings"), nsnull, aURI,
>+                                 nsnull, principal, nsnull, getter_AddRefs(domdoc));

This doesn't do the right thing.  It creates a document which has mLoadedAsData true and mLoadedAsInteractiveData false, whereas you want the former false and the latter true for XBL.  I suggest creating an NS_NewXBLDocument method in nsXMLDocument.cpp which does the right thing directly...

>+  *aDocInfo = NS_NewXBLDocumentInfo(doc);

Doing that and then returning error rv is not that great.  Probably better to store the new documentinfo in an nsRefPtr on the stack here for now (and make NS_NewXBLDocumentInfo return alread_AddRefed to make this simpler) and then store in *aDocInfo before the NS_OK return at the end of the method.

I don't see where you're setting mScriptAccess and mIsChrome.  Don't you need to do that?

>+nsXBLDocumentInfo::WritePrototypeBindings()

>+  rv = NewBufferFromStorageStream(storageStream, getter_Transfers(buf), &len);

Can you please add documentation to NewBufferFromStorageStream saying that *buffer is allocated with new[]?

>+  return startupCache->PutBuffer(PromiseFlatCString(spec).get(), buf, len);

|spec| is already flat.  No need for the PromiseFlatCString bit.

>+++ b/content/xbl/src/nsXBLDocumentInfo.h
>-  nsIURI* DocumentURI() { return mDocument->GetDocumentURI(); }
>+  nsIURI* DocumentURI() { return mDocument ? mDocument->GetDocumentURI() : nsnull; }

That's supposed to be an infallible getter; the name says so.  That said, how can it possibly become fallible with your new code?  I don't see a way it can, so why this change?

>+++ b/content/xbl/src/nsXBLProtoImpl.cpp
>+nsXBLProtoImpl::Read(nsIScriptContext* aContext,

>+  JSContext *cx = (JSContext *)aContext->GetNativeContext();

static_cast, please.

>+  ::JS_SetVersion(cx, JSVERSION_LATEST);

Why do you need that, exactly?  Shouldn't it already be there?

>+        // XXXndeakin do we need to handle multiple constructors or destructors?

No, you do not.


>+++ b/content/xbl/src/nsXBLProtoImplField.cpp
>+nsXBLProtoImplField::Write(nsIScriptContext* aContext, nsIObjectOutputStream* aStream)
>+  if (mFieldText) {
>+    rv = aStream->WriteWStringZ(mFieldText);
>+  }
>+  else {
>+    rv = aStream->Write32(0);

Write EmptyString.get() in the !mFieldText case, please.

>+++ b/content/xbl/src/nsXBLProtoImplMethod.cpp
>+#include "jsobj.h"

I believe that the JS folks are not exporting this header anymore; you shouldn't need it here anyway.

>+nsXBLProtoImplMethod::Write(nsIScriptContext* aContext,
>+  rv = aStream->Write32(0); // XXXndeakin write out line number

XBL_SerializeFunction should handle this.

>+nsXBLProtoImplAnonymousMethod::Write(nsIScriptContext* aContext,
>+    rv = aStream->Write32(0); // XXXndeakin write out line number

Again, XBL_SerializeFunction should handle this.


>+++ b/content/xbl/src/nsXBLProtoImplProperty.cpp
>+nsXBLProtoImplProperty::Write(nsIScriptContext* aContext,
>+    rv = aStream->Write32(0); // XXXndeakin write out line number

Yes, please.  But shouldn't XBL_SerializeFunction handle this (and take the line number as argument)?  The assymmetry betweeb XBL_SerializeFunction and XBL_DeserializeFunction is unfortunate.

>+++ b/content/xbl/src/nsXBLPrototypeBinding.cpp

>+nsXBLPrototypeBinding::Read(nsIObjectInputStream* aStream,
>+  rv = NS_NewURI(getter_AddRefs(mBindingURI), bindingURI, "", nsnull);

Just drop those two last args, if you're not going to pass non-default values.

>+  nsCAutoString baseURI;

baseBindingURI.

>+    rv = NS_NewURI(getter_AddRefs(mBaseBindingURI), baseURI, "", nsnull);

Again, drop the last two args.

>+  if (baseTag.Length()) {

!baseTag.IsEmpty(), please.

>+  nsCAutoString id;
>+  url->GetRef(id);
>+  NS_ENSURE_TRUE(!id.IsEmpty(), NS_ERROR_FAILURE);

Please document that this mirrors the code that doesn't create a binding if id is empty in nsXBLContentSink::ConstructBinding.  And also add a comment there pointing to this code, in case someone decides to change this.

>+  nsNodeInfoManager* nim = aDocument->NodeInfoManager();
>+  nsCOMPtr<nsINodeInfo> nodeInfo =
>+    nim->GetNodeInfo(nsGkAtoms::binding, nsnull, kNameSpaceID_XBL,
>+                     nsIDOMNode::ELEMENT_NODE);
>+  NS_NewElement(getter_AddRefs(mBinding), kNameSpaceID_XBL, nodeInfo.forget(),
>+                mozilla::dom::NOT_FROM_PARSER);

Why not just use aDocument->CreateElem() here?  If you want to avoid atomizing the tag name for performance reasons, we should just add an nsIAtom version of CreateElem, imo.

>+  nsCOMPtr<nsIContent> child;
>+  rv = ReadContentNode(aStream, aDocument, nim, getter_AddRefs(child));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (child) {
>+    mBinding->AppendChildTo(child, false);
>+  }
>+
>+  Element* rootElement = aDocument->GetRootElement();
>+  if (rootElement)
>+    rootElement->AppendChildTo(mBinding, false);

Why append |child| to mBinding before appending mBinding to |rootElement|?  Doing it the other way should be faster... (e.g. only requires one recursive treewalk over |child| and its descendants).

>+  if (!mInterfaceTable) {

How could it possibly _not_ be null here?  Can we assert it's null?

Also, should we really be creating mInterfaceTable if interfaceCount == 0 (which is the common case)?  I don't think we do.  Fix that, please.

>+  for (; interfaceCount > 0; interfaceCount--) {
>+    nsIID iid;
>+    aStream->ReadID(&iid);

This looks bad to me.  In particular, the binding specifies interfaces by _name_, whereas this code is saving/reading the actual IID.  Which means that if the IID for a given name changes, we will suddenly have a broken binding when deserializing.  That seems suboptimal.

>+    mInterfaceTable->Put(&key, mBinding);

Can you please file a followup bug about making mInterfaceTable into a hash set, since that's what it really wants to be, as far as I can tell?

>+  bool isFirstBinding = aFlags & XBLBinding_Serialize_IsFirstBinding;
>+  rv = Init(id, aDocInfo, nsnull, isFirstBinding);

I _think_ this works, but the Init() call will set mBindingURI and mAlternateBindingURI.  Why did we set them above, exactly?

I would think given this that we should _not_ serialize/deserialize mBindingURI/mAlternateBindingURI and just let Init() handle setting them up.  Any reason not to do that?

>+  rv = aDocInfo->SetPrototypeBinding(id, this);
>+  NS_ENSURE_SUCCESS(rv, rv);

Document here that this needs to happen before we start trying to deserialize the nsXBLProtoImpl?

>+    NS_NewXBLProtoImpl(this, NS_ConvertUTF8toUTF16(className).get(), &mImplementation);

NS_NewXBLProtoImpl already sets mImplementation, no?  I'd rather pass a pointer to the stack for the third arg with a comment about that...

>+    // Otherwise, there is no implementation, however, we need to check if
>+    // there are any handlers or resources.

I don't understand how this works.  nsXBLPrototypeBinding::Write() writes out the list starting with mPrototypeHandler unconditionally.  It also writes out mResources unconditionally.  Both are done after calling mImplementation->Write.

But the read side calls mImplementation->Read when there is an implementation, and only reads the mPrototypeHandler/mResources if there isn't one.  That doesn't match what was written at all.

Am I just missing something?  Is the points that nsXBLProtoImpl::Write does NOT write out things that are stored in mResources and mPrototypeHandler but nsXBLProtoImpl::Read _does_ read them?  If so, why do it that way?  It would be better, imo, to have parallel code between Read and Write without the magic....

>+GatherInsertionPoints(nsHashKey *aKey, void *aData, void* aClosure)
>+{
>+  ArrayOfInsertionPointsByContent* insertionPointsByContent =
>+    (ArrayOfInsertionPointsByContent *)aClosure;
>+
>+  nsXBLInsertionPointEntry* entry = (nsXBLInsertionPointEntry *)aData;

static_cast, please.

>+  // Add a new blank array if it isn't already there.
>+  nsTArray<InsertionItem>* list;
>+  if (!insertionPointsByContent->Get(entry->GetInsertionParent(), &list)) {
>+    list = new nsTArray<InsertionItem>;

This list will never have less that one entry in it right?  Should be an nsAutoTArray<1>?  Presumably both here and in the definition of ArrayOfInsertionPointsByContent.

>+  // Add the item to the array and ensure it stays sorted by insertion index.
>+  nsIAtom* atom = (nsIAtom *)((nsISupportsKey *)aKey)->GetValue();

static_cast, please.

>+WriteInterfaceID(nsHashKey *aKey, void *aData, void* aClosure)
>+{
>+  nsID iid = ((nsIIDKey *)aKey)->mKey;
>+  ((nsIObjectOutputStream *)aClosure)->WriteID(iid);

static_cast, but again I'm not sure this is a reasonable way to serialize the IIDs.

>+nsXBLPrototypeBinding::Write(nsIObjectOutputStream* aStream)
>+  // mAlternateBindingURI is set on the first binding to the binding url
>+  // without the ref. If set, then this is the first binding in a file.

Why not just check XBLDocumentInfo()->GetPrototypeBinding(EmptyCString()) == this ?  So:

  if (XBLDocumentInfo()->GetPrototypeBinding(EmptyCString()) == this) {
    flags |= XBLBinding_Serialize_IsFirstBinding;
  }

Or even just test that mAlternateBindingURI is non-null.  That should be equivalent, right?

>+    // Write an end marker when there is no more content.
>+    rv = aStream->Write8(XBLBinding_Serialize_NoMoreContent);

It's not that there is no more content; it's that there is nothing to serialize/deserialize.  This seems to be a consistent usage for XBLBinding_Serialize_NoMoreContent.  Perhaps it should be called XBLBinding_Serialize_NoContent with a comment indicating that it means that no content node should be created when this is the namespace?

>+    // Write out an empty classname. This indicates that the binding does not
>+    // define an implementation.
>+    rv = aStream->Write32(0);

This should be writing an actual empty string, since it plans to read a string.

I don't see anything here writing or reading mKeyHandlersRegistered or mKeyHandlers.  Why is that OK?  If it is, please document why.

>+nsXBLPrototypeBinding::ReadContentNode(nsIObjectInputStream* aStream, nsIDocument* aDocument,

We could use way more documentation (in the header, perhaps) about the format that nodes are serialized in.

>+  if (prefix.Length())

  if (!prefix.IsEmpty())

>+  if (namespaceID == kNameSpaceID_XUL) {

Please document here and in the XBL sink that this code needs to match?  Wish thre were a good way to share it, but I don't see one.

>+    nsCOMPtr<nsIURI> documentURI = aDocument->GetDocumentURI();

This can be just nsIURI*, right?

>+      attrs = new nsXULPrototypeAttribute[attrCount];
>+      NS_ENSURE_TRUE(attrs, NS_ERROR_OUT_OF_MEMORY);

That's an infallible allocation, so attrs will never be null.

>+    for (PRUint32 i = 0; i < attrCount; i++) {
....
>+      else {
>+        nsCOMPtr<nsIAtom> prefixAtom = do_GetAtom(prefix);

This assumes prefix is nonempty.  I believe this assumption may not be true if the attribute was set via setAttributeNS; it can well have no prefix in that case.

Just check for nonempty prefix before the do_GetAtom call, please.

>+    NS_NewElement(getter_AddRefs(content), nodeInfo->NamespaceID(),
>+                  ni.forget(), mozilla::dom::FROM_PARSER_NETWORK);

FROM_PARSER_NETWORK is wrong unless you actually plan to duplicate all the DoneCreatingElement and DoneAddingChildren magic the parser does.  Which I really hope you're not.  Please pass NOT_FROM_PARSER instead.

>+      if (prefix.Length())

  if (!prefix.IsEmpty())

>+    if (!mAttributeTable) {
>+      mAttributeTable = new nsObjectHashtable(nsnull, nsnull,
>+                                              DeleteAttributeTable,
>+                                              nsnull, 4);
>+    }

This might be good to move to an EnsureAttributeTable that also gets called from ConstructAttributeTable.

>+    nsPRUint32Key nskey(srcNamespaceID);

And this part should probably be factored out between here and ConstructAttributeTable.  It looks pretty identical to me.

>+    nsXBLInsertionPointEntry* xblIns = nsXBLInsertionPointEntry::Create(content);

So that creates xblIns with a zero refcount.  And then any early return before it's added to the hashtable leaks it.  Please use an nsRefPtr here?

>+WriteAttribute(nsHashKey *aKey, void *aData, void* aClosure)
>+  WriteAttributeData* data = (WriteAttributeData *)aClosure;

static_cast.

>+  nsXBLAttributeEntry* entry = (nsXBLAttributeEntry *)aData;

And here.

>+      nsAutoString name;
>+      entry->GetSrcAttribute()->ToString(name);
>+      stream->WriteWStringZ(name.get());

  stream->WriteWStringZ(nsDependentAtomString(entry->GetSrcAttribute()).get());

>+      entry->GetDstAttribute()->ToString(name);
>+      stream->WriteWStringZ(name.get());

  stream->WriteWStringZ(nsDependentAtomString(entry->GetDstAttribute()).get());

>+WriteAttributeNS(nsHashKey *aKey, void *aData, void* aClosure)
>+  WriteAttributeData* data = (WriteAttributeData *)aClosure;
>+  data->srcNamespace = ((nsPRUint32Key *)aKey)->GetValue();

static_cast for both.

>+nsXBLPrototypeBinding::WriteContentNode(nsIObjectOutputStream* aStream,

This would be better named WriteElement, since it assumes it's only called on elements...  The other option is to keep it as WriteContentNode, but move the checking of what sort of node it is to the top of this function to better parallel the way ReadContentNode works.

>+  nsAutoString tag;
>+  aNode->Tag()->ToString(tag);
>+  rv = aStream->WriteWStringZ(tag.get());

  rv = aStram->WriteWStringZ(nsDependentAtomString(aNode->Tag()).get());

Similar for other places where you write atoms unconditionally (e.g. attribute localnames).

In fact, it might be good to share the code that writes out (namespace, prefix, localname) instead of duplicating it.  Reading is more complicated because of the need to switch on the namespace, of course.

>+    nsAutoString val;
>+    aNode->GetAttr(attr->NamespaceID(), localName, val);
>+    rv = aStream->WriteWStringZ(val.get());

This is actually somewhat broken: no reason |val| can't contain embedded nulls.

Maybe leave it like this for now and file a followup on fixing (by adding a better string-writing API to nsIBinaryOutputStream)?

>+  rv = aStream->Write8(XBLBinding_Serialize_NoMoreAttributes);

Add documentation where that constant is defined explaining what it means, please.
>+      // The list is sorted by insertionIndex so all items pertaining to
>+      // this point will be in a group. For the first one in the group,
>+      // write out the default content and the number in the group.

Why is the default content guaranteed to be on the first item of the group?  Just because each item with the same insertionIndex actually corresponds to the same nsXBLInsertionPointEntry*?  If so, that should be documented here.

>+      nsAutoString tag;
>+      list->ElementAt(l).tag->ToString(tag);
>+      rv = aStream->WriteWStringZ(tag.get());

nsDependentAtomString, as usual.

>+  rv = aStream->Write32(XBLBinding_Serialize_NoMoreInsertionPoints);

Again, document this constant in the header?

>+    nsINode* child = aNode->GetChildAt(i);

Make |child| an nsIContent*.  Then you won't need the casts below.  GetChildAt() returns nsIContent*.

>+      rv = aStream->WriteWStringZ(content.get());

Again, nothing says there can't be null chars in the text.  We really need to fix this in the binary stream.

>+      // Ignore this type of node by simply writting out an end marker. This could
>+      // be a comment node, for instance.

Why is this OK?  Won't it break scripts that depend on child offsets?

>+nsXBLPrototypeBinding::WriteNamespace(nsIObjectOutputStream* aStream, PRInt32 aNameSpaceID)

>+  // XBL. If an other namespace is used however, the namespace id will be

s/an other/another/

>+++ b/content/xbl/src/nsXBLPrototypeBinding.h
>+// insertion points. It contains comparision operators so that it can be stored

"comparison"

>+  bool operator<(const InsertionItem& item) const
>+  bool operator==(const InsertionItem& item) const

In both of these, if this->insertionIndex == item.insertionIndex, can we assert that the defaultContent matches too?

>+typedef nsClassHashtable<nsISupportsHashKey, nsTArray<InsertionItem> > ArrayOfInsertionPointsByContent;

Again, this may want to be a nsAutoTArray.

The new methods here could really use some documentation.

>+nsXBLPrototypeHandler::nsXBLPrototypeHandler(nsXBLPrototypeBinding* aBinding)
>+  ++gRefCnt;
>+  if (gRefCnt == 1)
>+    // Get the primary accelerator key.
>+    InitAccessKeys();

Could we factor this out into a small function called from both constructors, please?

>+  nsAutoString handlerText;
>+  rv = aStream->ReadString(handlerText);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  mHandlerText = ToNewUnicode(handlerText);

I don't see how this can work when !mHandlerText when writing.  We should just check for the empty string here.

>+nsXBLPrototypeHandler::Write(nsIScriptContext* aContext, nsIObjectOutputStream* aStream)
>+  nsAutoString eventName;
>+  mEventName->ToString(eventName);
>+  rv = aStream->WriteWStringZ(eventName.get());

nsDependentAtomString.

>+  if (mHandlerText) {
>+    rv = aStream->WriteWStringZ(mHandlerText);
>+  }
>+  else {
>+    rv = aStream->Write32(0);

Shouldn't this last explicitly write out an empty string using EmptyString().ge()?

>+++ b/content/xbl/src/nsXBLSerialize.h

>+// A version number to ensure we don't load cached data in a different
>+// file format.
>+#define XBLBinding_Serialize_Version 0x37880001

Why not 1 instead?  If there is a good reason, please document what it is and
why this value.

>+#define XBLBinding_Serialize_InheritStyle 2
>+#define XBLBinding_Serialize_NoMoreBindings 0x80

Document, please.

Also document that XBLBinding_Serialize_Mask needs to cover all member types.
Comment 46 Tony Mechelynck [:tonymec] 2011-10-26 00:06:35 PDT
(In reply to Boris Zbarsky (:bz) from comment #45)
[...]
> Another meta-comment: when you address the review comments, I'd love to see
> an interdiff in addition to the new patch version.
[...]

The interdiff can be obtained from Bugzilla: view the attachment as diff, and in the header there is an item, "Differences between [ rolldown widget | v ] and this patch [ Diff ]". Select the other attachment in the rolldown widget, then click the "Diff" button, and Bugzilla gives you the interdiff automagically.
Comment 47 Boris Zbarsky [:bz] 2011-10-26 00:14:16 PDT
> The interdiff can be obtained from Bugzilla

If you get lucky, and the moon is in the right phase, and most importantly if none of the files have changed between when the original patch was posted and the new one was posted.

And even then I've seen Bugzilla's interdiff silently leave parts of the diff out, which means that for practical purposes it's useless because you never know when it's lying to you.
Comment 48 Jan Varga [:janv] 2011-10-26 00:42:29 PDT
(In reply to Boris Zbarsky (:bz) from comment #47)
> > The interdiff can be obtained from Bugzilla
> 
> If you get lucky, and the moon is in the right phase, and most importantly
> if none of the files have changed between when the original patch was posted
> and the new one was posted.
> 
> And even then I've seen Bugzilla's interdiff silently leave parts of the
> diff out, which means that for practical purposes it's useless because you
> never know when it's lying to you.

I have the same experience. You can't rely on it.
I just use |hg diff| before |hg qrefresh| to get interdiffs.
Comment 49 Neil Deakin 2011-10-27 06:09:45 PDT
(In reply to Boris Zbarsky (:bz) from comment #45)
> Comment on attachment 563725 [details] [diff] [review] [diff] [details] [review]

> There was no check for a system principal when writing bindings out.  There should be one.  Chrome/resource URIs may not have system principals, last I checked.

Can you clarify this? Do you mean that I should check for and only serialize those with system principals?

> I don't see where you're setting mScriptAccess and mIsChrome.  Don't you need to do that?

Aren't these set in the nsXBLDocumentInfo constructor?

> I don't see anything here writing or reading mKeyHandlersRegistered or mKeyHandlers.  Why is that OK?  If it is, please document why.

If mKeyHandlersRegistered is false, then GetKeyEventHandlers is called to initialize both.

>>+      // Ignore this type of node by simply writting out an end marker. This could
>>+      // be a comment node, for instance.
> Why is this OK?  Won't it break scripts that depend on child offsets?

Which other types of nodes do I need to support (besides comments) here?

> This looks bad to me.  In particular, the binding specifies interfaces by _name_, whereas this code is saving/reading the actual IID.  Which means that if the IID for a given name changes, we will suddenly have a broken binding when deserializing.  That seems suboptimal.

The IID would only change if one was running a different build. If so, the entire cache would be invalidated anyway.

But I can see if this is possible without a lot of extra work.
Comment 50 Boris Zbarsky [:bz] 2011-10-27 06:36:36 PDT
(In reply to Neil Deakin from comment #49)
> Can you clarify this? Do you mean that I should check for and only serialize
> those with system principals?

Yes.

> Aren't these set in the nsXBLDocumentInfo constructor?

Indeed.  Good.  :)

> If mKeyHandlersRegistered is false, then GetKeyEventHandlers is called to
> initialize both.

Great.  Add a comment in the serialization function saying they don't need to be serialized because they're created lazily?

> Which other types of nodes do I need to support (besides comments) here?

Comments and CDATA nodes should be good enough, I think.  Luckily, those should be pretty similar to text.

> The IID would only change if one was running a different build. If so, the
> entire cache would be invalidated anyway.

Agreed about different build.  What would force the cache invalidation in that situation?  If we guarantee that, then there's no reason to change this code; just add a comment pointing out the mechanism that makes it ok.
Comment 51 Neil Deakin 2011-10-27 07:52:52 PDT
(In reply to Boris Zbarsky (:bz) from comment #50)
> Agreed about different build.  What would force the cache invalidation in
> that situation?  If we guarantee that, then there's no reason to change this
> code; just add a comment pointing out the mechanism that makes it ok.

The last version a profile was loaded with is stored in <profile>/compatibility.ini. On startup, this is checked at http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#3242 and if the versions don't match, the startup cache files are deleted. The extension manager also invalidates the cache when extensions have changed.
Comment 52 Boris Zbarsky [:bz] 2011-10-27 10:37:04 PDT
Perfect.  Just add a comment about that!
Comment 53 Neil Deakin 2011-10-31 18:08:00 PDT
Created attachment 570894 [details] [diff] [review]
Part 4.4 - serialize/deserialize bindings to startup cache

Address comments
Comment 54 Neil Deakin 2011-10-31 18:10:39 PDT
Created attachment 570895 [details] [diff] [review]
Part 5.1 - serialize/deserialize property and method line numbers
Comment 55 Neil Deakin 2011-10-31 18:12:26 PDT
(In reply to Neil Deakin from comment #51)
> The last version a profile was loaded with is stored in
> <profile>/compatibility.ini. On startup, this is checked at
> http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.
> cpp#3242 and if the versions don't match, the startup cache files are
> deleted. The extension manager also invalidates the cache when extensions
> have changed.

Whoops, forgot to put this comment in. I'll add it to the beginning of one of the read methods.
Comment 56 Neil Deakin 2011-10-31 18:17:17 PDT
Created attachment 570896 [details] [diff] [review]
Patch differences

This is the difference between the old patch and the new. You might also consider https://hg.mozilla.org/try/rev/efce974c2b52 which contains just the differences caused by the review comments, as opposed to other unrelated changes (such as bool conversion).
Comment 57 Boris Zbarsky [:bz] 2011-11-01 12:44:03 PDT
Comment on attachment 570895 [details] [diff] [review]
Part 5.1 - serialize/deserialize property and method line numbers

I don't understand this patch.

Should't XBL_DeserializeFunction set the line number on the function it produces?

And if XDR handles that already, why do we need to do anything explicit with line numbers at all?
Comment 58 Boris Zbarsky [:bz] 2011-11-01 12:46:43 PDT
> You might also consider https://hg.mozilla.org/try/rev/efce974c2b52

Yes, that's more like what I was looking for.
Comment 59 Boris Zbarsky [:bz] 2011-11-01 13:29:49 PDT
Comment on attachment 570894 [details] [diff] [review]
Part 4.4 - serialize/deserialize bindings to startup cache

>+++ b/content/xbl/src/nsXBLDocumentInfo.cpp
>+nsXBLDocumentInfo::WritePrototypeBindings()
>+  nsCOMPtr<nsIPrincipal> principal;
>+  nsContentUtils::GetSecurityManager()->GetSystemPrincipal(getter_AddRefs(principal));
>+  if (principal != mDocument->NodePrincipal())

  if (!nsContentUtils::IsSystemPrincipal(mDocument->NodePrincipal())

>+++ b/content/xbl/src/nsXBLProtoImpl.cpp
>+nsXBLProtoImpl::Read(nsIScriptContext* aContext,
>+        NS_ASSERTION(false, "Unexpected binding member type");

NS_ERROR, please.

>+++ b/content/xbl/src/nsXBLPrototypeBinding.cpp
>+nsXBLPrototypeBinding::Read(nsIObjectInputStream* aStream,
>+  // Next read in the handlers.
...
>+    if (type == XBLBinding_Serialize_NoMoreItems)
>+      break;

I think we should either error out if 

  (type & XBLBinding_Serialize_Mask) != XBLBinding_Serialize_Handler

or at least assert that they're equal.

The rest of this while loop is mis-indented.  It needs to be outdented by 2 spaces.

>+  // Finally, read in the resources.

>+    rv = aStream->Read8(&type);
>+    NS_ASSERTION(type == XBLBinding_Serialize_Stylesheet ||
>+                 type == XBLBinding_Serialize_Image, "invalid resource type");

Do we not need to mask the type against XBLBinding_Serialize_Mask here?

r=me with the above nits fixed.  This looks great!
Comment 60 Boris Zbarsky [:bz] 2011-11-01 13:35:18 PDT
Oh, and thank you _very_ much for the try changeset link.  That made the followup review a a _lot_ easier.
Comment 61 Neil Deakin 2011-11-02 17:14:30 PDT
Created attachment 571517 [details] [diff] [review]
Part 5.2 - just remove line number handling

For some reason, I thought there were some getters that retrieved the line numbers. This patch just removes the writing and reading of line numbers, as the dxr process does that for us.
Comment 62 Boris Zbarsky [:bz] 2011-11-02 19:04:15 PDT
Comment on attachment 571517 [details] [diff] [review]
Part 5.2 - just remove line number handling

Ah, ok.  r=me

Let's get this landed!
Comment 65 :Ms2ger 2011-11-06 13:59:29 PST
Created attachment 572333 [details] [diff] [review]
Part 6: Cleanup

Some unused code, some unnecessary casts, some superfluous '::'s, some incorrect copyright assignments, some C-style casts and a build warning fixed just for you.
Comment 66 Stefan 2011-11-13 00:32:06 PST
(In reply to Ed Morley [:edmorley] from comment #64)
> https://hg.mozilla.org/mozilla-central/rev/b46ffd95bfd8

This patch causes the loss of controls in audio/video element:
https://bugzilla.mozilla.org/show_bug.cgi?id=701662#c7
Comment 67 :Ms2ger 2011-11-16 10:22:00 PST
Landed

https://hg.mozilla.org/mozilla-central/rev/b832a0860c4b

for Gecko 11.
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 14:43:00 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=701662#c24
This needs to be backed out (trunk and aurora) for causing bug 701662, since there's no motion on that bug.
Comment 69 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-12-18 14:44:17 PST
Is there a simple way to just disable it without removing the code?
Comment 70 Neil Deakin 2011-12-18 17:43:52 PST
It can be easily be disabled by commenting out the calls to nsXBLDocumentInfo::ReadPrototypeBindings and nsXBLDocumentInfo::WritePrototypeBindings within nsXBLService.cpp
Comment 71 Alex Keybl [:akeybl] 2012-01-05 12:28:03 PST
bug 701662 was fixed in 10/11

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