Closed Bug 592943 Opened 10 years ago Closed 9 years ago

switch nsXULPrototypeCache to use startupCache instead of fastload

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: benedict, Assigned: mwu)

References

Details

Attachments

(7 files, 4 obsolete files)

See patches in bug 520309.
Assignee: nobody → bhsieh
Reposting the patch from bug 520309.
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.
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.
Assignee: bhsieh → tglek
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.
Attached patch Updated patch (obsolete) — Splinter Review
Patch unbitrotted and cleaned up a bit. I removed ifdef LIBXUL and unindented some stuff.
Assignee: tglek → mwu
Attachment #473776 - Attachment is obsolete: true
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)
Attachment #528189 - Flags: review?(dtownsend) → review+
This is probably the last failing test. Running on try right now.
Attachment #529591 - Flags: review?(joshmoz)
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+
Blocks: 654489
Comment on attachment 528187 [details] [diff] [review]
Updated patch

This looks right to me as well.
Attachment #528187 - Flags: review?(jst) → review+
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
http://hg.mozilla.org/mozilla-central/rev/eaa69ae330ab
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 660374
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]
Will cs a3b5d768fa96 fix this?
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
Depends on: 660389
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
unbitrotted.
Attachment #528187 - Attachment is obsolete: true
Some minor fixes to the original patch.
Attachment #538410 - Flags: review?(jst)
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)
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)
This makes PathifyURI also handle chrome URIs.
Attachment #538415 - Flags: review?(mh+mozilla)
Blocks: 648125
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?
(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.
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 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.
(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 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 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+
(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.
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 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+
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.
Switch to recursion for all cases.
Attachment #539701 - Attachment is obsolete: true
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)
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+
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: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Depends on: 665390
<3
Depends on: 806175
You need to log in before you can comment on or make changes to this bug.