Last Comment Bug 650494 - Remove nsIXULPrototypeCache
: Remove nsIXULPrototypeCache
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Marco Castelluccio [:marco]
:
Mentors:
Depends on:
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2011-04-16 06:25 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-08-27 01:49 PDT (History)
7 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removed nsIXULPrototypeCache (16.31 KB, patch)
2011-08-02 17:53 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Review
Removed nsIXULPrototypeCache v2 (15.92 KB, patch)
2011-08-05 10:58 PDT, Marco Castelluccio [:marco]
bugs: review+
khuey: review-
Details | Diff | Review
Removed nsIXULPrototypeCache v3 (18.29 KB, patch)
2011-08-13 18:28 PDT, Marco Castelluccio [:marco]
khuey: review+
Details | Diff | Review
Remove nsIXULPrototypeCache v4 (19.24 KB, patch)
2011-08-25 19:00 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Review
Remove nsIXULPrototypeCache v5 (19.22 KB, patch)
2011-08-26 09:42 PDT, Marco Castelluccio [:marco]
no flags Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-04-16 06:25:02 PDT
54 /**
55  * This interface lets code from outside gklayout access the prototype cache.
56  */

That's no longer necessary.
Comment 1 Marco Castelluccio [:marco] 2011-08-02 17:53:50 PDT
Created attachment 550266 [details] [diff] [review]
Removed nsIXULPrototypeCache
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-02 18:20:18 PDT
Comment on attachment 550266 [details] [diff] [review]
Removed nsIXULPrototypeCache

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

Looks pretty good.  A few comments:

::: content/xul/document/public/nsIXULPrototypeCache.h
@@ -84,5 @@
> -// bytecode version changes.
> -#define XUL_FASTLOAD_FILE_VERSION       (0xfeedbeef - 25)
> -
> -#define XUL_SERIALIZATION_BUFFER_SIZE   (64 * 1024)
> -#define XUL_DESERIALIZATION_BUFFER_SIZE (8 * 1024)

Are these constants not used anywhere?

::: content/xul/document/src/nsXULPrototypeCache.cpp
@@ +107,5 @@
>  }
>  
>  
>  NS_IMPL_THREADSAFE_ISUPPORTS2(nsXULPrototypeCache,
> +                              nsISupports,

You don't need to explicitly list nsISupports.  You should change this to

NS_IMPL_THREADSAFE_ISUPPORTS1(nsXULPrototypeCache, nsIObserver)

We could probably drop the THREADSAFE bit too, but there's no need to do that here.

@@ +119,5 @@
>      if (aOuter)
>          return NS_ERROR_NO_AGGREGATION;
>  
>      nsRefPtr<nsXULPrototypeCache> result = new nsXULPrototypeCache();
> +    if (!result)

Don't add in these irrelevant whitespace changes here.

@@ +162,2 @@
>      if (!sInstance) {
> +        sInstance = new nsXULPrototypeCache();

You will need to AddRef this, and Release it somewhere appropriate (from nsLayoutStatics::Shutdown, perhaps).

::: content/xul/document/src/nsXULPrototypeCache.h
@@ +72,5 @@
>   * The cache has two levels:
>   *  1. In-memory hashtables
>   *  2. The on-disk cache file.
>   */
> +class nsXULPrototypeCache : public nsISupports,

Again, no need to explicitly list nsISupports.
Comment 3 Marco Castelluccio [:marco] 2011-08-05 10:58:31 PDT
Created attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

Added AddRef in nsXULPrototypeCache::GetInstance.
Release is yet done in nsXULPrototypeCache::ReleaseGlobals, called by nsLayoutStatics::Shutdown.
The constant weren't used anywhere (at least this is what mxr says).

If you could, please review this today or tomorrow, because Sunday I'll set off on a journey.
Comment 4 Marco Castelluccio [:marco] 2011-08-06 17:29:21 PDT
khuey if the patch I've presented is good, could you checkin it? I won't be here until 14 August, I wouldn't like in the meantime it will be bitrotted
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-07 07:24:32 PDT
Comment on attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

>+        sInstance->AddRef();

This should be NS_ADDREF(sInstance);

Other than that this looks good to me.  I'm not a peer of this stuff though, so I'm going to ask smaug to do the official review.
Comment 6 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2011-08-07 08:35:08 PDT
Comment on attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

> nsXULPrototypeCache::GetInstance()
> {
>-    // Theoretically this can return nsnull and callers should handle that.
>     if (!sInstance) {
>-        nsIXULPrototypeCache* cache;
>-
>-        CallGetService(kXULPrototypeCacheCID, &cache);
>-
>-        sInstance = static_cast<nsXULPrototypeCache*>(cache);
>+        sInstance = new nsXULPrototypeCache();
>+        sInstance->AddRef();
NS_ADDREF(sInstance = new nsXULPrototypeCache());


>-class nsXULPrototypeCache : public nsIXULPrototypeCache,
>-                                   nsIObserver
>+class nsXULPrototypeCache : public nsIObserver
> {
> public:
>     // nsISupports
>     NS_DECL_ISUPPORTS
>     NS_DECL_NSIOBSERVER
> 
>     // nsIXULPrototypeCache
Drop this comment


>     virtual PRBool IsCached(nsIURI* aURI) {
You can make the methods which were defined in nsIXULPrototypeCache non-virtual.
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-12 12:56:51 PDT
Comment on attachment 551085 [details] [diff] [review]
Removed nsIXULPrototypeCache v2

Actually, this patch has a very serious bug.  There's a lot of logic in NS_NewXULPrototypeCache which is no longer getting run.  The browser won't start with this patch ...
Comment 8 Marco Castelluccio [:marco] 2011-08-13 06:51:38 PDT
NS_NewXULPrototypeCache seems unused. It was used only in nsLayoutModule.cpp, but with the patch we've removed this use.
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-13 09:17:41 PDT
(In reply to Marco Castelluccio from comment #8)
> NS_NewXULPrototypeCache seems unused. It was used only in
> nsLayoutModule.cpp, but with the patch we've removed this use.

Yes, and that's the problem.  Before your patch, the prototype cache was created through XPCOM, which called NS_NewXULPrototypeCache.  That runs the code starting at http://mxr.mozilla.org/mozilla-central/source/content/xul/document/src/nsXULPrototypeCache.cpp#128 which initializes the prototype cache, sets up observers, etc.  With your patch, we don't create the prototype cache through that function, so this code never gets run.

You need to move that logic in NS_NewXULPrototypeCache into the construction case nsXULPrototypeCache::GetInstance.  It'd actually probably neater to split the hashtable initializations into nsXULPrototypeCache's constructor and put the rest in NS_NewXULPrototypeCache, but that's the minimum that's necessary.

You can test this pretty easily, with your patch as is the browser doesn't even start.
Comment 10 Marco Castelluccio [:marco] 2011-08-13 18:28:13 PDT
Created attachment 552916 [details] [diff] [review]
Removed nsIXULPrototypeCache v3

Moved the XULPrototypeCache creation from NS_NewXULPrototypeCache to the constructor. Removed NS_NewXULPrototypeCache.

I haven't done the checks for the memory, because the cache is created at the start, so they should be useless.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-15 05:15:19 PDT
Comment on attachment 552916 [details] [diff] [review]
Removed nsIXULPrototypeCache v3

This looks good.  I'll throw it at the tryserver before landing.
Comment 12 Mozilla RelEng Bot 2011-08-15 10:30:44 PDT
Try run for 1297abb8c3c6 is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=1297abb8c3c6
Results (out of 167 total builds):
    exception: 2
    success: 124
    warnings: 41
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/khuey@mozilla.com-1297abb8c3c6
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-15 10:34:14 PDT
So, there's unfortunately still a problem here.

The addons manager tries to get the prototype cache service at:

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#964

It does this just to ensure that the prototype cache is running before it flushes.  I don't think we need this, because if the cache is not alive then there's nothing to flush.  CCing mwu to see if he knows.
Comment 14 Michael Wu [:mwu] 2011-08-22 18:46:03 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #13)
> I don't think we need this, because if the cache is not alive then
> there's nothing to flush.  CCing mwu to see if he knows.

Sounds about right to me.

This looks like a relic from when the prototype cache used fastload. Starting the prototype cache was necessary to clear fastload. Now that the prototype cache uses startupcache, we can clear everything the prototype cache uses without starting the prototype cache.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-23 07:36:16 PDT
OK.

Marco, you can just remove the line at http://hg.mozilla.org/mozilla-central/annotate/a41b781330a6/toolkit/mozapps/extensions/XPIProvider.jsm#l970 and run this through try again then.
Comment 16 Marco Castelluccio [:marco] 2011-08-25 19:00:57 PDT
Created attachment 555922 [details] [diff] [review]
Remove nsIXULPrototypeCache v4

Added the needed change. I've also sent the patch to try, it will comment here.
Comment 17 Mozilla RelEng Bot 2011-08-25 20:50:56 PDT
Try run for cf2ed2ac2e2d is complete.
Detailed breakdown of the results available here:
    http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=cf2ed2ac2e2d
Results (out of 170 total builds):
    exception: 2
    success: 153
    warnings: 15
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mar.castelluccio@studenti.unina.it-cf2ed2ac2e2d
Comment 18 Marco Castelluccio [:marco] 2011-08-26 09:42:20 PDT
Created attachment 556055 [details] [diff] [review]
Remove nsIXULPrototypeCache v5

Corrected commit message. Now this could be checked in, as the try build was successful.
Comment 19 Ed Morley [:emorley] 2011-08-26 10:06:42 PDT
In my queue, pushing to inbound once confirmed everything builds locally.
Comment 21 Marco Bonardo [::mak] 2011-08-27 01:49:21 PDT
http://hg.mozilla.org/mozilla-central/rev/728cf3529cba

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