Closed
Bug 592943
Opened 14 years ago
Closed 14 years ago
switch nsXULPrototypeCache to use startupCache instead of fastload
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: benedict, Assigned: mwu)
References
Details
Attachments
(7 files, 4 obsolete files)
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mwu
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
See patches in bug 520309.
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → bhsieh
Reporter | ||
Comment 1•14 years ago
|
||
Reposting the patch from bug 520309.
Reporter | ||
Comment 2•14 years ago
|
||
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.
Reporter | ||
Comment 3•14 years ago
|
||
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.
Reporter | ||
Updated•14 years ago
|
Assignee: bhsieh → tglek
Reporter | ||
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
Patch unbitrotted and cleaned up a bit. I removed ifdef LIBXUL and unindented some stuff.
Assignee: tglek → mwu
Attachment #473776 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
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.
Attachment #528189 -
Flags: review?(dtownsend)
Updated•14 years ago
|
Attachment #528189 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 7•14 years ago
|
||
This is probably the last failing test. Running on try right now.
Attachment #529591 -
Flags: review?(joshmoz)
Assignee | ||
Comment 8•14 years ago
|
||
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.
Attachment #528187 -
Flags: review?(jst)
Attachment #529591 -
Flags: review?(joshmoz) → review+
Comment 9•14 years ago
|
||
Comment on attachment 528187 [details] [diff] [review]
Updated patch
This looks right to me as well.
Attachment #528187 -
Flags: review?(jst) → review+
Assignee | ||
Comment 10•14 years ago
|
||
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
Assignee | ||
Comment 11•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•14 years ago
|
||
Some tabs got in.
http://hg.mozilla.org/mozilla-central/rev/a3b5d768fa96
Comment 13•14 years ago
|
||
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•14 years ago
|
||
Will cs a3b5d768fa96 fix this?
Comment 15•14 years ago
|
||
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•14 years ago
|
||
Assignee | ||
Comment 19•14 years ago
|
||
Some minor fixes to the original patch.
Attachment #538410 -
Flags: review?(jst)
Assignee | ||
Comment 20•14 years ago
|
||
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.
Attachment #538411 -
Flags: review?(tglek)
Assignee | ||
Comment 21•14 years ago
|
||
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.
Attachment #538414 -
Flags: review?(jst)
Assignee | ||
Comment 22•14 years ago
|
||
This makes PathifyURI also handle chrome URIs.
Attachment #538415 -
Flags: review?(mh+mozilla)
Comment 23•14 years ago
|
||
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?
Assignee | ||
Comment 24•14 years ago
|
||
(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.
Assignee | ||
Comment 25•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
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)
Attachment #538415 -
Flags: review?(mh+mozilla) → review-
Comment 29•14 years ago
|
||
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.
Attachment #538411 -
Flags: review?(tglek) → review+
Assignee | ||
Comment 30•14 years ago
|
||
(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.
Assignee | ||
Comment 31•14 years ago
|
||
Switched to recursion to resolve inner jars, eliminated .bin appending since we don't really need it, comment updated.
Attachment #538415 -
Attachment is obsolete: true
Attachment #539701 -
Flags: review?(mh+mozilla)
Comment 32•14 years ago
|
||
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?
Attachment #539701 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 33•14 years ago
|
||
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.
Assignee | ||
Comment 34•14 years ago
|
||
Switch to recursion for all cases.
Attachment #539701 -
Attachment is obsolete: true
Assignee | ||
Comment 35•14 years ago
|
||
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.
Attachment #538410 -
Flags: review?(jst)
Assignee | ||
Comment 36•14 years ago
|
||
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
Attachment #538414 -
Flags: review?(jst) → review+
Assignee | ||
Comment 37•14 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/743bc4458fc1
http://hg.mozilla.org/integration/mozilla-inbound/rev/8468c5916b75
http://hg.mozilla.org/integration/mozilla-inbound/rev/59d282cf2d86
Whiteboard: [inbound]
Assignee | ||
Comment 38•14 years ago
|
||
Merged into mozilla-central.
http://hg.mozilla.org/mozilla-central/rev/743bc4458fc1
http://hg.mozilla.org/mozilla-central/rev/8468c5916b75
http://hg.mozilla.org/mozilla-central/rev/59d282cf2d86
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Whiteboard: [inbound]
Updated•14 years ago
|
Target Milestone: --- → mozilla7
Reporter | ||
Comment 39•13 years ago
|
||
<3
You need to log in
before you can comment on or make changes to this bug.
Description
•