650494 - Remove nsIXULPrototypeCache

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

54 /**
55  * This interface lets code from outside gklayout access the prototype cache.
56  */

That's no longer necessary.

Comment 1

[Marco Castelluccio [:marco]]

Attachment: Removed nsIXULPrototypeCache

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

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

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 (Exited; not receiving bugmail, old account, do not use)]

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][[e-mail]]]

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 (Exited; not receiving bugmail, old account, do not use)]

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

NS_NewXULPrototypeCache seems unused. It was used only in nsLayoutModule.cpp, but with the patch we've removed this use.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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]]

Attachment: 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 (Exited; not receiving bugmail, old account, do not use)]

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]

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/[e-mail]-1297abb8c3c6

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

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

Attachment: Remove nsIXULPrototypeCache v4

Added the needed change. I've also sent the patch to try, it will comment here.

Comment 17

[Mozilla RelEng Bot]

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/[e-mail]-cf2ed2ac2e2d

Comment 18

[Marco Castelluccio [:marco]]

Attachment: Remove nsIXULPrototypeCache v5

Corrected commit message. Now this could be checked in, as the try build was successful.

Comment 19

[Ed Morley [:emorley]]

In my queue, pushing to inbound once confirmed everything builds locally.

Comment 20

[Ed Morley [:emorley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/728cf3529cba

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/728cf3529cba