switch nsXULPrototypeCache to use startupCache instead of fastload

RESOLVED FIXED in mozilla7

Status

()

Core
XPCOM
RESOLVED FIXED
7 years ago
4 months ago

People

(Reporter: benedict, Assigned: mwu)

Tracking

(Depends on: 1 bug)

unspecified
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 4 obsolete attachments)

(Reporter)

Description

7 years ago
See patches in bug 520309.
(Reporter)

Updated

7 years ago
Assignee: nobody → bhsieh
(Reporter)

Comment 1

7 years ago
Created attachment 473776 [details] [diff] [review]
client nsXULPrototypeCache, as before

Reposting the patch from bug 520309.
(Reporter)

Comment 2

7 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

7 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

7 years ago
Assignee: bhsieh → tglek
(Reporter)

Comment 4

7 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.
Blocks: 606076
(Assignee)

Comment 5

6 years ago
Created attachment 528187 [details] [diff] [review]
Updated patch

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

6 years ago
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.
Attachment #528189 - Flags: review?(dtownsend)
Attachment #528189 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 7

6 years ago
Created attachment 529591 [details] [diff] [review]
Make test_bug485118.xul less racy

This is probably the last failing test. Running on try right now.
Attachment #529591 - Flags: review?(joshmoz)
(Assignee)

Comment 8

6 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)

Updated

6 years ago
Attachment #529591 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

6 years ago
Blocks: 654489
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

6 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

6 years ago
http://hg.mozilla.org/mozilla-central/rev/eaa69ae330ab
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

6 years ago
Some tabs got in.

http://hg.mozilla.org/mozilla-central/rev/a3b5d768fa96
Depends on: 660374

Comment 13

6 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

6 years ago
Will cs a3b5d768fa96 fix this?

Comment 15

6 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

6 years ago
Opened bug https://bugzilla.mozilla.org/show_bug.cgi?id=660389
Depends on: 660389
(Assignee)

Comment 17

6 years ago
Backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 18

6 years ago
Created attachment 538409 [details] [diff] [review]
Updated patch, v2

unbitrotted.
Attachment #528187 - Attachment is obsolete: true
(Assignee)

Comment 19

6 years ago
Created attachment 538410 [details] [diff] [review]
Add license boilerplate and fix obviously wrong check

Some minor fixes to the original patch.
Attachment #538410 - Flags: review?(jst)
(Assignee)

Comment 20

6 years ago
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.
Attachment #538411 - Flags: review?(tglek)
(Assignee)

Comment 21

6 years ago
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.
Attachment #538414 - Flags: review?(jst)
(Assignee)

Comment 22

6 years ago
Created attachment 538415 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs

This makes PathifyURI also handle chrome URIs.
Attachment #538415 - Flags: review?(mh+mozilla)
(Assignee)

Updated

6 years ago
Blocks: 648125

Comment 23

6 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

6 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

6 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 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 29

6 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

6 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

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

Comment 33

6 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

6 years ago
Created attachment 539709 [details] [diff] [review]
Improve PathifyURI to handle chrome URIs, v3

Switch to recursion for all cases.
Attachment #539701 - Attachment is obsolete: true
(Assignee)

Comment 35

6 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

6 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

6 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

6 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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]

Updated

6 years ago
Target Milestone: --- → mozilla7
Depends on: 665390
(Reporter)

Comment 39

6 years ago
<3
Depends on: 806175
You need to log in before you can comment on or make changes to this bug.