Closed Bug 1166356 Opened 5 years ago Closed 5 years ago

XUL documents not being cached by nsXULPrototypeCache

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jchen, Assigned: jchen)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

We do some kind of double-caching detection at [1] that I don't think really works. The result is none of the XUL documents are being cached. I checked the startupcache file from my profile directory and there's no .xul files in the cache.

Note that line 431, "if (!mCacheURITable.GetEntry(uri)) {", contradicts line 439, "mCacheURITable.RemoveEntry(uri);", within the if block.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/xul/nsXULPrototypeCache.cpp?rev=3fb0310f5f49#431
The code that tries to detect double-caching in nsXULPrototypeCache never worked right (see comment 0). I don't think it hurts to remove it entirely.
Attachment #8610697 - Flags: review?(Jan.Varga)
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Hey :glandium, :nalexander mentioned you found some problems with XUL startup cache that's been happening for a long time? Does this look related?
Flags: needinfo?(mh+mozilla)
I'm not aware of such problems.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8610697 [details] [diff] [review]
Don't try to detect double-caching in nsXULPrototypeCache (v1)

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

It was long time ago I worked on this code. I remember that the XUL prototype cache is kept in memory and startupcache lives on disk. So your first comment doesn't make sense to me. I would rather ask someone else to review this patch.
Attachment #8610697 - Flags: review?(Jan.Varga)
Maybe Neil Deakin would know.
Comment on attachment 8610697 [details] [diff] [review]
Don't try to detect double-caching in nsXULPrototypeCache (v1)

Thanks! To clarify, nsXULPrototypeCache writes the in-memory cache to disk using startupcache, but it first uses mCacheURITable to check if it thinks the entry is already in startupcache. However, the logic is not quite right, and the result is we *never* write anything to startupcache. I think we should get rid of this logic and always try to write the cache to startupcache.
Attachment #8610697 - Flags: review?(enndeakin)
(In reply to Jim Chen [:jchen] [:darchons] from comment #6)
> Comment on attachment 8610697 [details] [diff] [review]
> Don't try to detect double-caching in nsXULPrototypeCache (v1)
> 
> Thanks! To clarify, nsXULPrototypeCache writes the in-memory cache to disk
> using startupcache, but it first uses mCacheURITable to check if it thinks
> the entry is already in startupcache. However, the logic is not quite right,
> and the result is we *never* write anything to startupcache. I think we
> should get rid of this logic and always try to write the cache to
> startupcache.

Sounds like a serious bug.
Comment on attachment 8610697 [details] [diff] [review]
Don't try to detect double-caching in nsXULPrototypeCache (v1)

This code looks to have been added by Ehsan in bug 815847, so we should probably ask him. Having the xul cache not working is a problem (and that bug mentions a notable performance impact that bug caused), but I suspect that this patch might cause the assertions the bug describes.
Attachment #8610697 - Flags: review?(enndeakin) → review?(ehsan)
Comment on attachment 8610697 [details] [diff] [review]
Don't try to detect double-caching in nsXULPrototypeCache (v1)

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

mCacheURITable is used to prevent calling PutEntry twice, which you're regressing in your patch...  This should be triggering the assertions that Neil mentioned.

I think I have a proper description of the bug and how to fix it in the patch comments below.

Last but not least, thanks for catching this.  It looks like I broke putting stuff into the startup cache completely which caused the massive regressions there.  Not sure why/how I dropped the ball there.  Sorry.

::: dom/xul/nsXULPrototypeCache.cpp
@@ -427,5 @@
>      rv = NewBufferFromStorageStream(storageStream, getter_Transfers(buf), 
>                                      &len);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> -    if (!mCacheURITable.GetEntry(uri)) {

With my suggestion below, this check should be correct.  It's basically trying to prevent calling PutBuffer on the same URL twice.

@@ -435,5 @@
> -            return NS_ERROR_NOT_AVAILABLE;
> -        rv = sc->PutBuffer(spec.get(), buf, len);
> -        if (NS_SUCCEEDED(rv)) {
> -            mOutputStreamTable.Remove(uri);
> -            mCacheURITable.RemoveEntry(uri);

For consistency with the code path below, I think you need to convert this into a PutEntry call.  (Although that's probably not strictly necessary since I don't think BeginCaching can be called with the same URL after this point, but it's better to add it now than to break the startup cache because of this again in the future.

@@ -596,5 @@
>      }
>  
> -    // Success!  Insert this URI into the mCacheURITable
> -    // and commit locals to globals.
> -    mCacheURITable.PutEntry(aURI);

I think you want to move this a bit further up inside the big |if (NS_FAILED(rv))| block, right after a PutEntry call succeeds.  This should add an entry to the hashtable when we add it to the startup cache.
Attachment #8610697 - Flags: review?(ehsan) → review-
Also it would be awesome if you could add a test for this...  It's terrible that this managed to stay broken for 2.5 years without anyone noticing!
Blocks: 815847
Keywords: regression
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #9)
> Comment on attachment 8610697 [details] [diff] [review]
> Don't try to detect double-caching in nsXULPrototypeCache (v1)
> 
> Review of attachment 8610697 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review! I don't remember seeing assertions before, but I put back the mCacheURITable checks in this patch. I also changed mCacheURITable to mStartupCacheURITable to make it more clear which cache we're talking about.

> @@ -435,5 @@
> > -            return NS_ERROR_NOT_AVAILABLE;
> > -        rv = sc->PutBuffer(spec.get(), buf, len);
> > -        if (NS_SUCCEEDED(rv)) {
> > -            mOutputStreamTable.Remove(uri);
> > -            mCacheURITable.RemoveEntry(uri);
> 
> For consistency with the code path below, I think you need to convert this
> into a PutEntry call.  (Although that's probably not strictly necessary
> since I don't think BeginCaching can be called with the same URL after this
> point, but it's better to add it now than to break the startup cache because
> of this again in the future.

Done.

> @@ -596,5 @@
> >      }
> >  
> > -    // Success!  Insert this URI into the mCacheURITable
> > -    // and commit locals to globals.
> > -    mCacheURITable.PutEntry(aURI);
> 
> I think you want to move this a bit further up inside the big |if
> (NS_FAILED(rv))| block, right after a PutEntry call succeeds.  This should
> add an entry to the hashtable when we add it to the startup cache.

I don't think we need a PutEntry call here, because we're only adding an information tag to the startupcache here, which is not associated with the target URI. I did add some mCacheURITable.Clear calls where we invalidate the cache. I'm still concerned about mCacheURITable getting out-of-sync when startupcache is invalidated elsewhere, but in my testing I didn't see an issue with that.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #10)
> Also it would be awesome if you could add a test for this...  It's terrible
> that this managed to stay broken for 2.5 years without anyone noticing!

I'll look into it! I don't see any existing tests for nsXULPrototypeCache so I'm not quite sure how to go about testing it.
Attachment #8610697 - Attachment is obsolete: true
Attachment #8623132 - Flags: review?(ehsan)
(In reply to Jim Chen [:jchen] [:darchons] from comment #11)
> Created attachment 8623132 [details] [diff] [review]
> Properly detect double-caching in nsXULPrototypeCache (v2)
> 
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #9)
> > Comment on attachment 8610697 [details] [diff] [review]
> > Don't try to detect double-caching in nsXULPrototypeCache (v1)
> > 
> > Review of attachment 8610697 [details] [diff] [review]:
> > -----------------------------------------------------------------
> 
> Thanks for the review! I don't remember seeing assertions before, but I put
> back the mCacheURITable checks in this patch. I also changed mCacheURITable
> to mStartupCacheURITable to make it more clear which cache we're talking
> about.

Thanks, that is a good idea!

> > @@ -596,5 @@
> > >      }
> > >  
> > > -    // Success!  Insert this URI into the mCacheURITable
> > > -    // and commit locals to globals.
> > > -    mCacheURITable.PutEntry(aURI);
> > 
> > I think you want to move this a bit further up inside the big |if
> > (NS_FAILED(rv))| block, right after a PutEntry call succeeds.  This should
> > add an entry to the hashtable when we add it to the startup cache.
> 
> I don't think we need a PutEntry call here, because we're only adding an
> information tag to the startupcache here, which is not associated with the
> target URI.

Ah, yeah that is actually a great point.  I missed that earlier.

> I did add some mCacheURITable.Clear calls where we invalidate
> the cache. I'm still concerned about mCacheURITable getting out-of-sync when
> startupcache is invalidated elsewhere, but in my testing I didn't see an
> issue with that.

Yeah, I thought about that too, but I can't really think of a way to avoid that.

> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #10)
> > Also it would be awesome if you could add a test for this...  It's terrible
> > that this managed to stay broken for 2.5 years without anyone noticing!
> 
> I'll look into it! I don't see any existing tests for nsXULPrototypeCache so
> I'm not quite sure how to go about testing it.

Thanks!
Attachment #8623132 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/11f5871b0a22
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.