WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
31171
[V8] Need better design to solve the duplicate error message reporting problem.
https://bugs.webkit.org/show_bug.cgi?id=31171
Summary
[V8] Need better design to solve the duplicate error message reporting problem.
Jian Li
Reported
2009-11-05 10:32:08 PST
We need to come up with better design to solve the duplicate error message reporting problem, instead of relying on checking ScriptExecutionContext to avoid duplicate messages.
Attachments
patch
(7.04 KB, patch)
2010-03-19 06:24 PDT
,
Yury Semikhatsky
no flags
Details
Formatted Diff
Diff
patch
(10.36 KB, patch)
2010-03-19 11:31 PDT
,
Yury Semikhatsky
no flags
Details
Formatted Diff
Diff
patch
(10.43 KB, patch)
2010-03-22 02:16 PDT
,
Yury Semikhatsky
pfeldman
: review+
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Yury Semikhatsky
Comment 1
2010-03-19 06:24:18 PDT
Created
attachment 51150
[details]
patch
Pavel Feldman
Comment 2
2010-03-19 06:30:32 PDT
Comment on
attachment 51150
[details]
patch This looks sane from the v8 message handling perspective. Could you get lg from the workers guys prior to landing?
Yury Semikhatsky
Comment 3
2010-03-19 11:31:43 PDT
Created
attachment 51173
[details]
patch - Added test - v8MessageHandler is not reentarable otherwise it gets into an infinite recursion if there is an exception in onerror handler
Jian Li
Comment 4
2010-03-19 12:19:18 PDT
> diff --git a/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > +static void v8MessageHandler(v8::Handle<v8::Message> message, v8::Handle<v8::Value> data) > +{ > + static bool isReportingException = false;
Do we need to say "static bool" since we are already in a static function?
> + // Exceptions that occur in error handler should be ignored since in that case
An extra space after "since".
> + // WorkerContext::reportException will send the exception to the worker object. > + if (isReportingException) > + return; > + isReportingException = true; > + > + String errorMessage = toWebCoreString(message->Get());
Do we want to check if the message is empty? Previously we got a reliability build break in chromium due to that the exception can be thrown without setting a message.
Yury Semikhatsky
Comment 5
2010-03-22 02:15:48 PDT
(In reply to
comment #4
)
> > diff --git a/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp b/WebCore/bindings/v8/WorkerContextExecutionProxy.cpp > > +static void v8MessageHandler(v8::Handle<v8::Message> message, v8::Handle<v8::Value> data) > > +{ > > + static bool isReportingException = false; > Do we need to say "static bool" since we are already in a static function?
Yes, because we want it to be a static variable.
> > + // Exceptions that occur in error handler should be ignored since in that case > An extra space after "since".
Fixed.
> > + // WorkerContext::reportException will send the exception to the worker object. > > + if (isReportingException) > > + return; > > + isReportingException = true; > > + > > + String errorMessage = toWebCoreString(message->Get()); > Do we want to check if the message is empty? Previously we got a reliability > build break in chromium due to that the exception can be thrown without setting > a message.
Previously the message was pulled from v8::TryCatch that has SetCaptureMessage method that allows to disable message capturing. In MessageHandler add with v8::AddMessageHandler the message handle can be empty only if the exception was thrown when v8 bootstrapper is active which should never happen in normal conditions that's why there is no IsEmpty() check in V8ConsoleMessage::hanler where uncaught exceptions from Page are reporte.d
Yury Semikhatsky
Comment 6
2010-03-22 02:16:19 PDT
Created
attachment 51271
[details]
patch
Yury Semikhatsky
Comment 7
2010-03-22 02:46:51 PDT
Committing to
http://svn.webkit.org/repository/webkit/trunk
... M LayoutTests/ChangeLog A LayoutTests/fast/workers/resources/worker-exception-in-timeout-callback.js M LayoutTests/fast/workers/worker-script-error-expected.txt M LayoutTests/fast/workers/worker-script-error.html M WebCore/ChangeLog M WebCore/bindings/v8/V8AbstractEventListener.cpp M WebCore/bindings/v8/V8Utilities.cpp M WebCore/bindings/v8/V8Utilities.h M WebCore/bindings/v8/WorkerContextExecutionProxy.cpp Committed
r56329
Eric Seidel (no email)
Comment 8
2010-03-24 14:31:09 PDT
Comment on
attachment 51150
[details]
patch Cleared Pavel Feldman's review+ from obsolete
attachment 51150
[details]
so that this bug does not appear in
http://webkit.org/pending-commit
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug