Bug 37661 - [v8] Bail out if fetching of Object.prototype fails
Summary: [v8] Bail out if fetching of Object.prototype fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-15 10:09 PDT by anton muhin
Modified: 2010-04-21 10:21 PDT (History)
2 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2010-04-15 10:17 PDT, anton muhin
no flags Details | Formatted Diff | Diff
Better ChangeLog (3.60 KB, patch)
2010-04-20 08:24 PDT, anton muhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description anton muhin 2010-04-15 10:09:42 PDT
[v8] Bail out if fetching of Object.prototype fails
Comment 1 anton muhin 2010-04-15 10:16:11 PDT
Most probably that's due to empty handle of the value to set---receiver and keys are checked many times on the path so we would die earlier.

I don't know exact reason why fetching Object.prototype fails.  Most probably some exception is thrown (if it were due to IsDeadCheck, we shouldn't get past first line of v8::SetHiddenPrototype).

If we have some logging mechanism, we could try to see if it's indeed the case: we could add TryCatch and store exception somewhere on the stack to have a chance to read it from minidump.

For now adding some more bailout checks, let's see if it could help.
Comment 2 anton muhin 2010-04-15 10:17:12 PDT
Created attachment 53446 [details]
Patch
Comment 3 Adam Barth 2010-04-17 13:33:37 PDT
Comment on attachment 53446 [details]
Patch

The ChangeLog doesn't explain why we're making this change.  You bug comments give some hint, but they don't explain what problem we're trying to solve.  Is there a crash?  Can we make a test case for it?
Comment 4 anton muhin 2010-04-19 05:34:44 PDT
Sorry, I should have been more explicit.

It's a crash in Chromium.  Sample stack trace:

0x025ec87a	 [chrome.dll	 - handles.cc:217]	v8::internal::SetProperty(v8::internal::Handle<v8::internal::JSObject>,v8::internal::Handle<v8::internal::String>,v8::internal::Handle<v8::internal::Object>,PropertyAttributes)
0x0265d733	 [chrome.dll	 - runtime.cc:3799]	v8::internal::Runtime::SetObjectProperty(v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,PropertyAttributes)
0x025e9bb5	 [chrome.dll	 - objects.cc:1416]	v8::internal::JSObject::SetPropertyPostInterceptor(v8::internal::String *,v8::internal::Object *,PropertyAttributes)
0x025ec9fa	 [chrome.dll	 - handles.cc:226]	v8::internal::SetProperty(v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,v8::internal::Handle<v8::internal::Object>,PropertyAttributes)
0x025c7cd2	 [chrome.dll	 - api.cc:2393]	v8::Object::SetHiddenValue(v8::Handle<v8::String>,v8::Handle<v8::Value>)
0x025cb321	 [chrome.dll	 - api.cc:2083]	v8::Object::Get(v8::Handle<v8::Value>)
0x025ccbff	 [chrome.dll	 - api.cc:3069]	v8::Context::Global()
0x01d8a5f0	 [chrome.dll	 - v8domwindowshell.cpp:518]	WebCore::V8DOMWindowShell::installHiddenObjectPrototype(v8::Handle<v8::Context>)
0x01d89ffe	 [chrome.dll	 - v8domwindowshell.cpp:289]	WebCore::V8DOMWindowShell::initContextIfNeeded()
0x01d8c66e	 [chrome.dll	 - v8proxy.cpp:279]	WebCore::V8Proxy::evaluateInIsolatedWorld(int,WTF::Vector<WebCore::ScriptSourceCode,0> const &,int)
0x01e89d98	 [chrome.dll	 - webframeimpl.cpp:736]	WebKit::WebFrameImpl::executeScriptInIsolatedWorld(int,WebKit::WebScriptSource const *,unsigned int,int)
0x01ffd199	 [chrome.dll	 - user_script_slave.cc:219]	UserScriptSlave::InjectScripts(WebKit::WebFrame *,UserScript::RunLocation)
0x01fee20c	 [chrome.dll	 - render_view.cc:2658]	RenderView::didFinishDocumentLoad(WebKit::WebFrame *)
0x01e943db	 [chrome.dll	 - frameloaderclientimpl.cpp:391]	WebKit::FrameLoaderClientImpl::dispatchDidFinishDocumentLoad()
0x01c6c37e	 [chrome.dll	 - frameloader.cpp:1076]	WebCore::FrameLoader::finishedParsing()
0x01cc243a	 [chrome.dll	 - document.cpp:4203]	WebCore::Document::finishedParsing()
0x01e401a8	 [chrome.dll	 - htmlparser.cpp:1666]	WebCore::HTMLParser::finished()
0x01dc66a7	 [chrome.dll	 - htmltokenizer.cpp:1870]	WebCore::HTMLTokenizer::end()
0x01dc654f	 [chrome.dll	 - htmltokenizer.cpp:1811]	WebCore::HTMLTokenizer::write(WebCore::SegmentedString const &,bool)
0x01dc661d	 [chrome.dll	 - htmltokenizer.cpp:1848]	WebCore::HTMLTokenizer::timerFired(WebCore::Timer<WebCore::HTMLTokenizer> *)
0x01e07a0f	 [chrome.dll	 - timer.h:98]	WebCore::Timer<WebCore::Scrollbar>::fired()
0x01d2334b	 [chrome.dll	 - threadtimers.cpp:112]	WebCore::ThreadTimers::sharedTimerFiredInternal()
0x01d232be	 [chrome.dll	 - threadtimers.cpp:90]	WebCore::ThreadTimers::sharedTimerFired()
0x01fc2b6f	 [chrome.dll	 - message_loop.cc:329]	MessageLoop::RunTask(Task *)
0x01fc2bac	 [chrome.dll	 - message_loop.cc:337]	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0x01fc2d42	 [chrome.dll	 - message_loop.cc:444]	MessageLoop::DoWork()
0x01fd321f	 [chrome.dll	 - message_pump_default.cc:50]	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x01fc2a1a	 [chrome.dll	 - message_loop.cc:205]	MessageLoop::RunInternal()
0x01fc299f	 [chrome.dll	 - message_loop.cc:177]	MessageLoop::RunHandler()
0x01fc294d	 [chrome.dll	 - message_loop.cc:155]	MessageLoop::Run()
0x01fdd2fe	 [chrome.dll	 - renderer_main.cc:289]	RendererMain(MainFunctionParams const &)
0x01c33bb9	 [chrome.dll	 - chrome_dll_main.cc:716]	ChromeMain
0x004033ec	 [chrome.exe	 - client_util.cc:195]	MainDllLoader::Launch(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *)
0x00403a72	 [chrome.exe	 - chrome_exe_main.cc:46]	wWinMain
0x00445dce	 [chrome.exe	 - crt0.c:263]	__tmainCRTStartup
0x7c816fd6	 [kernel32.dll	 + 0x00016fd6]	BaseProcessStart

The analysis still apply: it's almost for sure empty handle of value being set.

Alas, I don't have an easy repro case.
Comment 5 Adam Barth 2010-04-19 14:08:16 PDT
Comment on attachment 53446 [details]
Patch

Ok. Please add more details to the change log before landing.
Comment 6 anton muhin 2010-04-20 08:24:53 PDT
Created attachment 53822 [details]
Better ChangeLog
Comment 7 anton muhin 2010-04-20 08:27:51 PDT
(In reply to comment #6)
> Created an attachment (id=53822) [details]
> Better ChangeLog

Adam, is this wording enough?
Comment 8 Adam Barth 2010-04-20 12:22:05 PDT
Comment on attachment 53822 [details]
Better ChangeLog

It's still pretty weak.  I don't want to hold up your patch longer, but someone looking at this change in the future won't understand why we made the change without referring to this bug.
Comment 9 anton muhin 2010-04-21 04:40:55 PDT
Comment on attachment 53822 [details]
Better ChangeLog

Thanks a lot, Adam
Comment 10 WebKit Commit Bot 2010-04-21 10:21:46 PDT
Comment on attachment 53822 [details]
Better ChangeLog

Clearing flags on attachment: 53822

Committed r57991: <http://trac.webkit.org/changeset/57991>
Comment 11 WebKit Commit Bot 2010-04-21 10:21:51 PDT
All reviewed patches have been landed.  Closing bug.