Summary: | XSS Auditor severely affects loading performance after submitting a large form | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Major | CC: | abarth, adele, dbates, sam, tonikitoo | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | 53200, 53205, 53266, 53279, 53336, 53338, 53339, 53345, 53354, 53362, 53364, 53365, 53368, 53370, 53635, 53640, 53643, 53652, 53662, 53664, 53665, 53741, 53750 | ||||||||
Bug Blocks: | 53405 | ||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2010-11-19 17:11:17 PST
If we cached the tree better, would that fix the issue? The memory impact of having more than one copy of the tree doesn't appear negligible. Even one copy of a tree for a 170K form is probably not negligible. We're seeing other reports of this slowness with internal website. Having a test case would be helpful here. I can try to create one based on the description in Comment #0. It seems like a reasonable first step is to improve the caching of the suffix tree. Created attachment 79330 [details]
test case
I can't easily share or even access actual data from the internal web applications, but here is a synthetic test case. Works fast in Firefox, of course.
Thanks Alexey. The proximate cause is that the CachingURLCanonicalizer isn't sufficiently associative. Alternating inline event handlers and inline script flips the decodeEntities bit back and forth, causing the cache to thrash. A short-term fix is to make the cache more associative. A long-term fix is to move to a new XSS detection architecture, which I've wanted to do for a while... I'm going to try for the short term fix first. We talked on IRC. I'm going to try the long-term fix first. I'll post incremental patches blocking this bug. With the patches up through Bug 53665, this test case is now blazingly fast. Created attachment 81176 [details]
Patch
I'll add the PerformanceTest in a follow-up patch. Comment on attachment 81176 [details]
Patch
r=me
Some discussion in IRC: [7:50pm] dydz: abarth: Some of the expected results have more console messages than before... [7:50pm] abarth: yeah [7:50pm] abarth: that's because we block injected "type" attributes [7:51pm] abarth: for objects now too [7:51pm] abarth: in XSSAuditor, you could inject an object tag for Gears [7:51pm] abarth: for example [7:51pm] abarth: because that doesn't result in a load [7:51pm] abarth: whereas XSSFilter will block that [7:52pm] abarth: dydz: i think there's some kind of attack scenario there involving a plugin with a non-standard way of loading stuff [7:52pm] abarth: dydz: but i don't have a concrete example Committed r77588: <http://trac.webkit.org/changeset/77588> > Of course, one can disable XSS Auditor with HTTP headers as a temporary workaround.
Ugh, it turns that one cannot do that with WebKit as released with shipping Safari. XSSAuditor::findInRequest() only checks for X-XSS-Protection header after performing all time consuming operations!
(In reply to comment #15) > > Of course, one can disable XSS Auditor with HTTP headers as a temporary workaround. > > Ugh, it turns that one cannot do that with WebKit as released with shipping Safari. XSSAuditor::findInRequest() only checks for X-XSS-Protection header after performing all time consuming operations! That's unfortunate. :( The new design shouldn't have that problem... |