Remove nsIXULPrototypeCache

RESOLVED FIXED in mozilla9

Status

()

Core
XUL
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: marco)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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

That's no longer necessary.

Updated

6 years ago
Assignee: nobody → 46b
(Assignee)

Comment 1

6 years ago
Created attachment 550266 [details] [diff] [review]
Removed nsIXULPrototypeCache
Attachment #550266 - Flags: review?(khuey)
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.

Updated

6 years ago
Assignee: 46b → mar.castelluccio
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 3

6 years ago
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.
Attachment #550266 - Attachment is obsolete: true
Attachment #550266 - Flags: review?(khuey)
Attachment #551085 - Flags: review?(khuey)
(Assignee)

Comment 4

6 years ago
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 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.
Attachment #551085 - Flags: review?(khuey)
Attachment #551085 - Flags: review?(Olli.Pettay)
Attachment #551085 - Flags: feedback+

Comment 6

6 years ago
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.
Attachment #551085 - Flags: review?(Olli.Pettay) → review+
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 ...
Attachment #551085 - Flags: feedback+ → review-
(Assignee)

Comment 8

6 years ago
NS_NewXULPrototypeCache seems unused. It was used only in nsLayoutModule.cpp, but with the patch we've removed this 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.
(Assignee)

Comment 10

6 years ago
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.
Attachment #551085 - Attachment is obsolete: true
Attachment #552916 - Flags: review?(khuey)
Comment on attachment 552916 [details] [diff] [review]
Removed nsIXULPrototypeCache v3

This looks good.  I'll throw it at the tryserver before landing.
Attachment #552916 - Flags: review?(khuey) → review+

Comment 12

6 years ago
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
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

6 years ago
(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.
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.
(Assignee)

Comment 16

6 years ago
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.
Attachment #552916 - Attachment is obsolete: true

Comment 17

6 years ago
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
(Assignee)

Comment 18

6 years ago
Created attachment 556055 [details] [diff] [review]
Remove nsIXULPrototypeCache v5

Corrected commit message. Now this could be checked in, as the try build was successful.
Attachment #555922 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
In my queue, pushing to inbound once confirmed everything builds locally.
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/728cf3529cba
Flags: in-testsuite-
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/728cf3529cba
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.