Whenever we load a new layout test, we do a navigation, which causes us to do a lot of brain transplant work. Each brain transplant requires us to re-wrap a given JS object. Doing so forces us to do some security checks so we know what kind of wrapper to use. This eventually boils down to these two lines in WrapperFactory.cpp: bool originSubsumesTarget = AccessCheck::subsumes(origin, target); bool targetSubsumesOrigin = AccessCheck::subsumes(target, origin); Blake showed me how, for non-system principals, these checks eventually call NS_SecurityCompareURIs. For file: URLs, this function checks if the two files are the same. The equality check appears to touch the file system on Windows because it's extremely slow--much slower than on Linux or Mac. Blake also told me that we can skip this file equality check by setting the security.fileuri.strict_origin_policy preference while running reftests. When I tried this, the running time for Win7 and WinXP debug reftests drops from ~100 minutes to ~70 minutes. The time for crashtests doesn't seem nearly as affected by this change. I'm not sure why. I think the main question here is: is it okay to set this preference? It seems to cause one test to fail (filter-extref-differentOrigin-01.svg) and it also causes a shutdown leak. I'll need to track these down.

This patch sets the pref. jsreftests already sets it, so this only affects layout tests and crashtests.

Setting the pref causes us to do this XBL check more often, which is slow. Bobby has a patch that will fix it, but it's not ready to land yet. This patch moves the XBL check so that it's called less often.

It looks like filter-extref-differentOrigin-01.svg is designed to fail in this situation. It's testing to make sure that a certain resource is inaccessible. Is there anything we can do here?

(In reply to Bill McCloskey (:billm) from comment #3) > It looks like filter-extref-differentOrigin-01.svg is designed to fail in > this situation. It's testing to make sure that a certain resource is > inaccessible. Is there anything we can do here? Just flip the pref for that reftest? I'm pretty sure reftest.list has a syntax for that.

This seems to work. I considered using --extra-profile-file to set the pref like we do for js reftests. However, it seems like we would want to do this for all reftests--crashtests also speed up by about 25%.

This test requires the pref to be true.

This fixes the leak I mentioned earlier.

Comment on attachment 722585 [details] [diff] [review] patch to set the pref, with comment Please put this in reftest-cmdline.js next to the ~15 other pref settings, rather than here (which is only used for information that needs to be communicated to the reftest harness from runreftest.py, which isn't really a manadatory component of reftest). r=dbaron with that (it'll be a setBoolPref call just like the others next to it)

Does this remove the need to use e.g. "HTTP(..)" annotations in reftests? (to give them access to resources like fonts / filters in other directories)

If so, it might be worth filing a followup on removing those annotations*, if we're expecting this bug's patches to stick, since they presumably won't be doing any good & reftest-authors won't bother to use those annotations going forward. * (except for any annotations on tests are explicitly testing something about HTTP urls, rather than just asking for access to local resources in other directories.)

Comment on attachment 722586 [details] [diff] [review] annotate differentOrigin test >diff --git a/layout/reftests/svg/reftest.list b/layout/reftests/svg/reftest.list >-fails-if(Android||B2G) == filter-extref-differentOrigin-01.svg pass.svg # Bug 695385 >+fails-if(Android||B2G) pref(security.fileuri.strict_origin_policy,true) == filter-extref-differentOrigin-01.svg pass.svg # Bug 695385 Could you add a comment above this line explaining why we're setting the pref? Maybe something along the lines of: # This pref is normally on by default, but we turn it off in reftest runs to # disable an unnecessary security-check. This reftest is actually testing that # the security check works, though, so it needs the pref to be turned on: r=me with an explanatory comment added.

(In reply to Daniel Holbert [:dholbert] from comment #9) > Does this remove the need to use e.g. "HTTP(..)" annotations in reftests? > (to give them access to resources like fonts / filters in other directories) I'm not familiar with this stuff, but it sounds like no. The documentation says: <http>, if present, is one of the strings (sans quotes) "HTTP" or "HTTP(..)" or "HTTP(../..)" or "HTTP(../../..)", etc. , indicating that the test should be run over an HTTP server because it requires certain HTTP headers or a particular HTTP status. So it sounds like tests use the HTTP annotation because they need HTTP behavior. Maybe you're asking whether HTTP(..) in particular can be eliminated, since it weakens restrictions on the path? I think the answer is no, since the pref applies only to URLs with the file scheme. It seems possible that some people are using HTTP solely because they want to access other files. In that case, we could eliminate the annotation from those tests. However, there are 362 tests with the HTTP annotation, so it seems impractical to do that in one big pass.

(In reply to Bill McCloskey (:billm) from comment #12) > It seems possible that some people are using HTTP solely because they want > to access other files. In that case, we could eliminate the annotation Based on my own experience and the uses of this annotation that I've seen, I think that's likely the majority of the uses. > However, there are 362 tests with the HTTP annotation, so it > seems impractical to do that in one big pass. Probably not as an automated pass, and probably not as a high-priority work item, agreed. But worth tracking in a followup bug (which e.g. a contributor could come along and take a crack at).

(In reply to Daniel Holbert [:dholbert] from comment #13) > Probably not as an automated pass, and probably not as a high-priority work > item, agreed. But worth tracking in a followup bug (which e.g. a contributor > could come along and take a crack at). OK, sounds good. I filed bug 849081 for that.

Comment on attachment 722591 [details] [diff] [review] fix leak with XULContentSinkImpl >+XULContentSinkImpl::ContextStack::Traverse(nsCycleCollectionTraversalCallback& cb) aCb >+{ >+ for (ContextStack::Entry *tmp = mTop; tmp; tmp = tmp->mNext) { ContextStack::Entry* tmp >+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(XULContentSinkImpl) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mNodeInfoManager) >+ tmp->mContextStack.Clear(); >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mDocumentURL) >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mPrototype) >+ NS_IF_RELEASE(tmp->mParser); >+ NS_IMPL_CYCLE_COLLECTION_UNLINK(mSecMan) >+NS_IMPL_CYCLE_COLLECTION_UNLINK_END >+ >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(XULContentSinkImpl) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNodeInfoManager) >+ tmp->mContextStack.Traverse(cb); >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mDocumentURL) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPrototype) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(mParser) >+ NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mSecMan) >+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END You need to traverse and unlink only mNodeInfoManager, mPrototype and mParser. mDocumentURL doesn't point to a cycle collectable object and neither does mSecMan I was looking at whether null checks are needed for those unlinked member variables, but I think no. (or otherwise were in very odd state)

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d2b2cdbed4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/215fdb623ab6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5820d183e2

A few tests are failing intermittently, so I backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/e38c5c346840

For the record, this did cut the running time of the Windows reftests roughly by half (?!?!).

I filed bug 849976 about the failing tests. It looks like they weren't testing what we expected them to test before my patch. I think it makes sense to fix them as a follow-up. This patch marks them as randomly failing and possibly asserting.

Comment on attachment 723641 [details] [diff] [review] annotate the failing tests as failing Stick the "# bug 849976" comments at the end of the line in the manifest, rather than on the previous line. (That's the common convention for how to annotate failing reftests w/ bug numbers.) r=me with that.

Comment on attachment 723641 [details] [diff] [review] annotate the failing tests as failing Actually -- John Daggett says the random failures and assertions are likely to go away if you annotate these tests as HTTP(..). Apparently we currently have issues loading local fonts via file:// URIs with the strict_origin_policy pref toggled off -- but he's working on fixing that -- bug 847272 tracks that. In the meantime, our behavior should become predictable if these tests are loaded over HTTP instead of file://. So -- instead of marking these as random asserts(0-1), please mark these tests as HTTP(..). (and once John's fixed that bug, these new HTTP(..) annotations should be able to go away as part of bug 849081.) So, r=me stands, if you change all the instances of... > # bug 849976 > random asserts(0-1) [stuff] ...in this patch with... > HTTP(..) [stuff] ...assuming that passes Try. (sorry for the additional Try-churn.)

Actually, it looks like jdaggett may be adding those HTTP(..) annotations over in bug 849976 (independently of this bug), which I think makes sense. That'll hopefully make attachment 723641 [details] [diff] [review] completely unnecessary.

Comment on attachment 723641 [details] [diff] [review] annotate the failing tests as failing [/me resets r+ to no-review on attachment 723641 [details] [diff] [review], since per the last few comments, I don't think we'll need it anymore]

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b313f1eab148 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bcb4a9e46df remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e2267744e3 Pushed without the final patch.

https://hg.mozilla.org/mozilla-central/rev/b313f1eab148 https://hg.mozilla.org/mozilla-central/rev/2bcb4a9e46df https://hg.mozilla.org/mozilla-central/rev/d6e2267744e3