New issue
Advanced search Search tips

Issue 597530 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

EmailInputType stores a ScriptRegexp in a static local keeping the regex context alive forever

Project Member Reported by esprehn@chromium.org, Mar 24 2016

Issue description

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/html/forms/EmailInputType.cpp&q=EmailInputType.cpp&sq=package:chromium&l=142

loyso@ in https://codereview.chromium.org/1360723003 made us forcibly clear the V8PerContextData in the regex scope when doing leak detection. This is to compensate for the fact that the static local makes the V8PerContextData live forever.

We should probably just not create the static local ScriptRegexp at all and instead create a new one each time. If we think creating the regex object is too expensive we can cache it as a property of the v8::Context itself, though v8 is supposed to cache regexes for us anyway, so in theory creating a new ScriptRegexp for each EmailInputType should be "cheap".

Doing this allows us to clear the regex context when under memory pressure, or periodically, since it's not used very often (only form validation and some random inspector things). Doing a single form validation should probably not cause a permanent memory increase the size of a v8::Context.
 
To start I'd just make it non-static and see how "slow" it is. I suspect it's not that bad.
Owner: tkent@chromium.org
Status: Assigned (was: Untriaged)
tkent-san: Would you mind taking a look at this?

Comment 3 by tkent@chromium.org, Jun 3 2016

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc3381f2aa9ccbb200f09d58ce34c22f7027491a

commit fc3381f2aa9ccbb200f09d58ce34c22f7027491a
Author: tkent <tkent@chromium.org>
Date: Mon Jun 06 04:34:10 2016

Make ScriptRegexp for email address a member of EmailInputType.

Make it a data member of EmailInputType instead of a function-local static variable.

- Move EmailInputType constructor implementation to the .cpp file to void to include
  ScriptRegexp.h in EmailInputType.h.
- Add ScriptRegexp arguments to convertEmailAddressToASCII() and
  isValidEmailAddress() and add createEmailRegexp() for ease of unit testing.

BUG= 597530 

Review-Url: https://codereview.chromium.org/2037963002
Cr-Commit-Position: refs/heads/master@{#397970}

[modify] https://crrev.com/fc3381f2aa9ccbb200f09d58ce34c22f7027491a/third_party/WebKit/Source/core/html/forms/EmailInputType.cpp
[modify] https://crrev.com/fc3381f2aa9ccbb200f09d58ce34c22f7027491a/third_party/WebKit/Source/core/html/forms/EmailInputType.h
[modify] https://crrev.com/fc3381f2aa9ccbb200f09d58ce34c22f7027491a/third_party/WebKit/Source/core/html/forms/EmailInputTypeTest.cpp

Comment 5 by tkent@chromium.org, Jun 6 2016

Status: Fixed (was: Started)

Sign in to add a comment