Last Comment Bug 592943 - switch nsXULPrototypeCache to use startupCache instead of fastload
: switch nsXULPrototypeCache to use startupCache instead of fastload
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla7
Assigned To: Michael Wu [:mwu]
:
:
Mentors:
Depends on: 806175 660374 660389 665390
Blocks: 606076 648125 654489
  Show dependency treegraph
 
Reported: 2010-09-01 21:36 PDT by Benedict Hsieh [:benedict]
Modified: 2012-11-05 06:30 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
client nsXULPrototypeCache, as before (58.29 KB, patch)
2010-09-09 15:34 PDT, Benedict Hsieh [:benedict]
no flags Details | Diff | Splinter Review
Updated patch (57.28 KB, patch)
2011-04-25 15:04 PDT, Michael Wu [:mwu]
jst: review+
Details | Diff | Splinter Review
Fix addon manager tests (8.33 KB, patch)
2011-04-25 15:07 PDT, Michael Wu [:mwu]
dtownsend: review+
Details | Diff | Splinter Review
Make test_bug485118.xul less racy (817 bytes, patch)
2011-05-02 15:59 PDT, Michael Wu [:mwu]
jaas: review+
Details | Diff | Splinter Review
Updated patch, v2 (56.95 KB, patch)
2011-06-09 18:14 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Add license boilerplate and fix obviously wrong check (2.61 KB, patch)
2011-06-09 18:15 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review
Move PathifyURI to StartupCacheUtils (11.69 KB, patch)
2011-06-09 18:25 PDT, Michael Wu [:mwu]
taras.mozilla: review+
Details | Diff | Splinter Review
Use NS_PathifyURI instead of GetPath (2.41 KB, patch)
2011-06-09 18:37 PDT, Michael Wu [:mwu]
mwu.code: review+
Details | Diff | Splinter Review
Improve PathifyURI to handle chrome URIs (1.91 KB, patch)
2011-06-09 18:42 PDT, Michael Wu [:mwu]
mh+mozilla: review-
Details | Diff | Splinter Review
Improve PathifyURI to handle chrome URIs, v2 (4.09 KB, patch)
2011-06-15 18:35 PDT, Michael Wu [:mwu]
mh+mozilla: review+
Details | Diff | Splinter Review
Improve PathifyURI to handle chrome URIs, v3 (3.61 KB, patch)
2011-06-15 19:32 PDT, Michael Wu [:mwu]
no flags Details | Diff | Splinter Review

Description Benedict Hsieh [:benedict] 2010-09-01 21:36:44 PDT
See patches in bug 520309.
Comment 1 Benedict Hsieh [:benedict] 2010-09-09 15:34:04 PDT
Created attachment 473776 [details] [diff] [review]
client nsXULPrototypeCache, as before

Reposting the patch from bug 520309.
Comment 2 Benedict Hsieh [:benedict] 2010-09-09 15:51:12 PDT
Also reposting:
These are the remaining failures on tryserver (with nsXULPrototypeCache patch
attached).

TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js
| no error while closing the WebConsole - Got true, expected false

a bit of investigation shows:
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/components/console/hudservice/tests/browser/browser_HUDServiceTestsAll.js | error: this.inputField is null @ chrome://global/content/bindings/textbox.xml 

---

ERROR TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | scrollbar
has wrong minimum width - got 0, expected 22
ERROR TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | Small
scrollbar has wrong minimum width - got 0, expected 19
ERROR TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | scrollbar
has wrong minimum height - got 0, expected 22
ERROR TEST-UNEXPECTED-FAIL |
chrome://mochikit/content/chrome/widget/tests/test_bug485118.xul | Small
scrollbar has wrong minimum height - got 0, expected 19

Haven't investigated these yet.
Comment 3 Benedict Hsieh [:benedict] 2010-09-09 16:15:06 PDT
The 
>TEST-UNEXPECTED-FAIL [snip]
>error: this.inputField is null @ chrome://global/content/bindings/textbox.xml

above was just a dummy test I added to send that output. That error message is reported to the window.onerror handler, so I imagine it's also what's causing the "error while closing WebConsole" failure, which is just an assertion that no errors happened.
Comment 4 Benedict Hsieh [:benedict] 2010-10-08 14:26:47 PDT
One thing to investigate is whether the xulPrototypeCache is caching things that use a non-system principal. I thought it didn't, but it seems that some tests might somehow make it do so.
Comment 5 Michael Wu [:mwu] 2011-04-25 15:04:02 PDT
Created attachment 528187 [details] [diff] [review]
Updated patch

Patch unbitrotted and cleaned up a bit. I removed ifdef LIBXUL and unindented some stuff.
Comment 6 Michael Wu [:mwu] 2011-04-25 15:07:15 PDT
Created attachment 528189 [details] [diff] [review]
Fix addon manager tests

Some of the tests need modification since we no longer use fastload for xul. We just make sure the addon manager sends the startupcache-invalidate notification when necessary. Other tests should cover whether or not startupcache-invalidate is handled correctly.
Comment 7 Michael Wu [:mwu] 2011-05-02 15:59:37 PDT
Created attachment 529591 [details] [diff] [review]
Make test_bug485118.xul less racy

This is probably the last failing test. Running on try right now.
Comment 8 Michael Wu [:mwu] 2011-05-02 16:08:21 PDT
Comment on attachment 528187 [details] [diff] [review]
Updated patch

jst, can you review this patch? Or is someone else more suited to review this?

This patch appears to pass all tests (once the test fixes are applied) and looks right to me. I didn't write this patch but I'll try to any address any review comments.
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-25 17:23:24 PDT
Comment on attachment 528187 [details] [diff] [review]
Updated patch

This looks right to me as well.
Comment 10 Michael Wu [:mwu] 2011-05-26 12:24:59 PDT
I pushed the test fixes. The actual patch will follow today or tomorrow.

http://hg.mozilla.org/mozilla-central/rev/464df55cbb29
http://hg.mozilla.org/mozilla-central/rev/7a4900cfb25a
Comment 11 Michael Wu [:mwu] 2011-05-27 11:41:44 PDT
http://hg.mozilla.org/mozilla-central/rev/eaa69ae330ab
Comment 12 Michael Wu [:mwu] 2011-05-27 13:20:29 PDT
Some tabs got in.

http://hg.mozilla.org/mozilla-central/rev/a3b5d768fa96
Comment 13 Gary [:streetwolf] 2011-05-27 16:56:55 PDT
After installing the hourly with this bug fix many of my addons do not work anymore.  I went back to the hourly before this fix and all addons are working.  I'm in the process of getting confirmation from others.

Possible regression:

Good :[url=http://hg.mozilla.org/mozilla-central/rev/02a5505b965b]mozilla-central: changeset 70257:02a5505b965b[/url]

Bad: [url=http://hg.mozilla.org/mozilla-central/rev/eaa69ae330ab]mozilla-central: changeset 70258:eaa69ae330ab[/url]
Comment 14 Gary [:streetwolf] 2011-05-27 16:59:19 PDT
Will cs a3b5d768fa96 fix this?
Comment 15 Gary [:streetwolf] 2011-05-27 17:25:04 PDT
Just installed cs c6d349c58bd7 which should include cs a3b5d768fa96. Many of my addons do not work.  If others can confirm addons don;t work this bug fix should be backed out asap.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110527 Firefox/7.0a1 - Build ID: 20110527132150
Comment 16 Gary [:streetwolf] 2011-05-27 17:37:57 PDT
Opened bug https://bugzilla.mozilla.org/show_bug.cgi?id=660389
Comment 17 Michael Wu [:mwu] 2011-05-28 00:18:20 PDT
Backed out.
Comment 18 Michael Wu [:mwu] 2011-06-09 18:14:12 PDT
Created attachment 538409 [details] [diff] [review]
Updated patch, v2

unbitrotted.
Comment 19 Michael Wu [:mwu] 2011-06-09 18:15:50 PDT
Created attachment 538410 [details] [diff] [review]
Add license boilerplate and fix obviously wrong check

Some minor fixes to the original patch.
Comment 20 Michael Wu [:mwu] 2011-06-09 18:25:58 PDT
Created attachment 538411 [details] [diff] [review]
Move PathifyURI to StartupCacheUtils

Moving pathifyuri so more users can use it. This also removes the hardcoding of the "jsloader" prefix and requires the user to provide the prefix if desired.
Comment 21 Michael Wu [:mwu] 2011-06-09 18:37:56 PDT
Created attachment 538414 [details] [diff] [review]
Use NS_PathifyURI instead of GetPath

This fixes collisions in cached entries due to the use of GetPath. /content/overlay.xul or something was a sufficiently common name that extensions could collide with browser xul startupcache entries. The use of NS_PathifyURI is much more resistant against collisions.
Comment 22 Michael Wu [:mwu] 2011-06-09 18:42:30 PDT
Created attachment 538415 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs

This makes PathifyURI also handle chrome URIs.
Comment 23 (dormant account) 2011-06-13 16:00:35 PDT
Comment on attachment 538411 [details] [diff] [review]
Move PathifyURI to StartupCacheUtils

So why does xul stuff need to pathify urls? Are you thinking of omnijaring that stuff too?
Comment 24 Michael Wu [:mwu] 2011-06-13 16:22:22 PDT
(In reply to comment #23)
> Comment on attachment 538411 [details] [diff] [review] [review]
> Move PathifyURI to StartupCacheUtils
> 
> So why does xul stuff need to pathify urls? Are you thinking of omnijaring
> that stuff too?

Yeah that's one of the nice properties of PathifyURI. I dunno if I'm going to do that yet, but this leaves the possibility while providing a good replacement for GetPath.
Comment 25 Michael Wu [:mwu] 2011-06-14 15:42:10 PDT
Here's some entries in my startup cache:

xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/browser.js
xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/cmd.js
xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/init.js
xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/interfaces.js
xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/overlay.xul
xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/prefs.js
xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/nosquint.jar!/content/zoommanager.js
xulcache/resource/gre/chrome/browser/content/browser/baseMenuOverlay.xul.bin
xulcache/resource/gre/chrome/browser/content/browser/browser.js.bin
xulcache/resource/gre/chrome/browser/content/browser/browser.xul.bin
xulcache/resource/gre/chrome/browser/content/browser/hiddenWindow.xul.bin
xulcache/resource/gre/chrome/browser/content/browser/macBrowserOverlay.xul.bin
xulcache/resource/gre/chrome/browser/content/browser/nsContextMenu.js.bin
xulcache/resource/gre/chrome/browser/content/browser/places/browserPlacesViews.js.bin
xulcache/resource/gre/chrome/browser/content/browser/places/controller.js.bin
xulcache/resource/gre/chrome/browser/content/browser/places/editBookmarkOverlay.js.bin
xulcache/resource/gre/chrome/browser/content/browser/places/placesOverlay.xul.bin
xulcache/resource/gre/chrome/browser/content/browser/places/treeView.js.bin
xulcache/resource/gre/chrome/browser/content/browser/safebrowsing/report-phishing-overlay.xul.bin
xulcache/resource/gre/chrome/browser/content/browser/safebrowsing/sb-loader.js.bin
xulcache/resource/gre/chrome/browser/content/browser/utilityOverlay.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/commonDialog.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/commonDialog.xul.bin
xulcache/resource/gre/chrome/toolkit/content/global/contentAreaUtils.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/editMenuOverlay.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/editMenuOverlay.xul.bin
xulcache/resource/gre/chrome/toolkit/content/global/globalOverlay.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/inlineSpellCheckUI.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/macWindowMenu.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/printUtils.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/viewSourceUtils.js.bin
xulcache/resource/gre/chrome/toolkit/content/global/viewZoomOverlay.js.bin
Comment 26 Mike Hommey [:glandium] 2011-06-15 00:47:36 PDT
Comment on attachment 538411 [details] [diff] [review]
Move PathifyURI to StartupCacheUtils

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

::: startupcache/StartupCacheUtils.cpp
@@ +175,5 @@
> + *     jsloader/resource/gre/modules/XPCOMUtils.jsm.bin
> + *  file://$PROFILE_DIR/extensions/{uuid}/components/component.js becomes
> + *     jsloader/$PROFILE_DIR/extensions/%7Buuid%7D/components/component.js.bin
> + *  jar:file://$PROFILE_DIR/extensions/some.xpi!/components/component.js becomes
> + *     jsloader/$PROFILE_DIR/extensions/some.xpi/components/component.js.bin

You need to adjust these comments for the fact that jsloader is now not handled by the NS_PathifyURI function.
Comment 27 Mike Hommey [:glandium] 2011-06-15 00:51:55 PDT
(In reply to comment #25)
> Here's some entries in my startup cache:
> 
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/browser.js
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/cmd.js
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/init.js
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/interfaces.js
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/overlay.xul
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/prefs.js
> xulcache/jar:jar:file:/Users/altape/Library/Application%20Support/Firefox/
> Profiles/0v2h2obt.default/extensions/nosquint@urandom.ca.xpi!/chrome/
> nosquint.jar!/content/zoommanager.js

Please note that that's not what pathifyuri currently does with jar urls for the startupcache.
Comment 28 Mike Hommey [:glandium] 2011-06-15 01:03:25 PDT
Comment on attachment 538415 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs

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

::: startupcache/StartupCacheUtils.cpp
@@ +251,5 @@
> +
> +                out.Append("/");
> +                out.Append(spec);
> +                return NS_OK;
> +            }

I think it would be better to recurse, here. Something like replacing
            nsCOMPtr<nsIFileURL> jarFileURL;
            jarFileURL = do_QueryInterface(jarFileURI, &rv);
            NS_ENSURE_SUCCESS(rv, rv);

            nsCAutoString path;
            rv = jarFileURL->GetPath(path);
            NS_ENSURE_SUCCESS(rv, rv);
            out.Append(path);
with
            NS_PathifyURI(jarFileURI, out)

might work (though would require to move the addition of the .bin suffix into the caller)
Comment 29 (dormant account) 2011-06-15 15:23:15 PDT
Comment on attachment 538411 [details] [diff] [review]
Move PathifyURI to StartupCacheUtils

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

Overall looks ok.

::: startupcache/StartupCacheUtils.h
@@ +64,5 @@
>  NS_NewBufferFromStorageStream(nsIStorageStream *storageStream, 
>                                char** buffer, PRUint32* len);
> +
> +NS_EXPORT nsresult
> +NS_PathifyURI(nsIURI *in, nsACString &out);

This kind of unrelated to the patch. But while you are at this can you get rid of NS_* prefix in this file? and Get rid of the using namespace mozilla::scache(didn't realize it was in a header too :( ) in mozJSComponentLoader?

also rename scache->startupcache and make all calls via the namespace ie

startupcache::PathifyURI.
Comment 30 Michael Wu [:mwu] 2011-06-15 15:55:59 PDT
(In reply to comment #27)
> Please note that that's not what pathifyuri currently does with jar urls for
> the startupcache.

That's because pathifyuri currently just gives up on these jar uris. Pathify uri can't handle inner jar uris.
Comment 31 Michael Wu [:mwu] 2011-06-15 18:35:04 PDT
Created attachment 539701 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs, v2

Switched to recursion to resolve inner jars, eliminated .bin appending since we don't really need it, comment updated.
Comment 32 Mike Hommey [:glandium] 2011-06-15 19:01:16 PDT
Comment on attachment 539701 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs, v2

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

::: startupcache/StartupCacheUtils.cpp
@@ +240,4 @@
>              rv = jarURI->GetJARFile(getter_AddRefs(jarFileURI));
>              NS_ENSURE_SUCCESS(rv, rv);
>  
> +            nsCAutoString path;

You can move that definition in the else part, however...

@@ +252,5 @@
>  
> +                rv = jarFileURL->GetPath(path);
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                out.Append(path);
> +            }

Doesn't recursing in every case work just as well?
Comment 33 Michael Wu [:mwu] 2011-06-15 19:11:37 PDT
Comment on attachment 539701 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs, v2

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

::: startupcache/StartupCacheUtils.cpp
@@ +240,4 @@
>              rv = jarURI->GetJARFile(getter_AddRefs(jarFileURI));
>              NS_ENSURE_SUCCESS(rv, rv);
>  
> +            nsCAutoString path;

path is used further down so I put it up here so the nsCAutoString gets reused.

@@ +252,5 @@
>  
> +                rv = jarFileURL->GetPath(path);
> +                NS_ENSURE_SUCCESS(rv, rv);
> +                out.Append(path);
> +            }

Hm, it should. Except for the overhead, it should work better. I'll try it.
Comment 34 Michael Wu [:mwu] 2011-06-15 19:32:59 PDT
Created attachment 539709 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs, v3

Switch to recursion for all cases.
Comment 35 Michael Wu [:mwu] 2011-06-15 20:04:22 PDT
Comment on attachment 538410 [details] [diff] [review]
Add license boilerplate and fix obviously wrong check

Skipping review on this since it's a trivial fix.
Comment 36 Michael Wu [:mwu] 2011-06-15 21:58:00 PDT
Comment on attachment 538414 [details] [diff] [review]
Use NS_PathifyURI instead of GetPath

[10:52:03]  <glandium> it's actually pretty straightforward, you're just using the startupcache with these pathified uris
[10:52:25]  <glandium> which is what you wrote earlier, but i know see it for myself :)
[10:52:35]  <mwu> yup yup
[10:53:09]  <glandium> so yep, i'm giving rubberstamp
[10:53:36]  <mwu> excellent
[10:54:36]  <glandium> i can even say r+, since i looked at the code
Comment 39 Benedict Hsieh [:benedict] 2011-06-27 06:44:31 PDT
<3

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