Summary: | Frequent crashes in JSC::parse in a worker thread when running regression tests | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, benjamin, ggaren, msaboff, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar, MakingBotsRed | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Enrica Casucci
2012-03-29 14:36:19 PDT
Created attachment 134844 [details]
Patch
Comment on attachment 134844 [details]
Patch
I feel like I've seen this patch before, and we decided that this was the wrong way to deal with empty source code. Is that not the case?
(In reply to comment #4) > Found it - the discussion was in bug 77593. I believe that bug 77593 handles the Null string case. This bug is to handle the empty string. Per our discussion, I will check to make sure that the normal script controller as well as the worker script controller both handle null and empty string the same. My understanding is that the empty string is intended to be a valid zero-length string, and all interfaces should accept it, while the NULL string is like a NULL pointer, it means "unspecified" or "uninitialized", and some interfaces accept it while some don't. If this understanding is true, it's definitely wrong to require clients of JavaScriptCore to short-circuit and avoid passing the empty string to the interpreter. As I've said in person, there are many ways a client could pass us the empty string, including through API. Changing this one place to paper over the problem is wrong. If this understanding is true, the fix in bug 77593 is also wrong: The result of executing an "unspecified" or "uninitialized" string as JavaScript is unknown, and it's likely that any client passing us such a thing has made a programming error. > If this understanding is true, the fix in bug 77593 is also wrong: The result of executing an "unspecified" or "uninitialized" string as JavaScript is unknown, and it's likely that any client passing us such a thing has made a programming error.
I do not disagree regarding 77593. I needed a quick fix so we reverted to the behavior existing before is8Bit() was added.
What you suggest would require more work but I think it is right.
Created attachment 135202 [details]
Updated patch
It turns out that isEmpty() works because it was checking both Null and empty. The issue we have here is the Null case. This patch uses a Null check in the caller to WorkerScriptController::evaluate() instead of having the isEmpty() check there.
Comment on attachment 135202 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=135202&action=review > Source/WebCore/workers/WorkerThread.cpp:153 > - script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL)); > + if (!m_startupData->m_sourceCode.isNull()) > + script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL)); With this, what will show up in the Inspector? My concern is it would show something fake starting for the current context instead of ignoring the previous context. Comment on attachment 135202 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=135202&action=review > Source/WebCore/workers/WorkerThread.cpp:153 > - script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL)); > + if (!m_startupData->m_sourceCode.isNull()) > + script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL)); With this, what will show up in the Inspector? My concern is it would show something fake starting for the current context instead of ignoring the previous context. Comment on attachment 135202 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=135202&action=review >>> Source/WebCore/workers/WorkerThread.cpp:153 >>> + if (!m_startupData->m_sourceCode.isNull()) >>> + script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL)); >> >> With this, what will show up in the Inspector? My concern is it would show something fake starting for the current context instead of ignoring the previous context. > > With this, what will show up in the Inspector? My concern is it would show something fake starting for the current context instead of ignoring the previous context. A better bottleneck for the NULL check is Worker::notifyFinished(). We should never start a worker thread with a NULL script. Once the worker has started, it should be able to assume that it has valid JavaScript to execute. Do you know why we end up with a NULL script and yet Worker::m_scriptLoader doesn't report that it failed to load? > A better bottleneck for the NULL check is Worker::notifyFinished(). We should never start a worker thread with a NULL script. Once the worker has started, it should be able to assume that it has valid JavaScript to execute.
>
> Do you know why we end up with a NULL script and yet Worker::m_scriptLoader doesn't report that it failed to load?
The common case I have seen goes like this:
1) page A schedule a thread
2) scripts change to page B
3) thread start on page B
-->loaded was invalidated, source is now null
I am sure there are other ways to invalidate the source, it is just what I have experienced.
Created attachment 135365 [details] Updated Patch > Do you know why we end up with a NULL script and yet Worker::m_scriptLoader doesn't report that it failed to load? WorkerScriptLoader::didReceiveData() allows for a null script to not be an error. It appears that the tests are checking the NULL case directly. Created attachment 135410 [details]
Final Patch
Updated patch that initializes m_script to an empty string so that even when no input is given, it will be processed as an empty string.
Reviewed in person by Geoff Garen.
Committed r113082: <http://trac.webkit.org/changeset/113082> |