Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1947213002:
Enable the MojoDOMSTorageBrowserTests (Closed)
DescriptionEnable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out.
BUG=586194, 609632, 608121
TBR=jam
Committed: https://crrev.com/6936a1c58d788c0f6ff847a139c0ff3918aa65b4
Cr-Commit-Position: refs/heads/master@{#392393}
Patch Set 1 #Patch Set 2 : disable on mac for now #Patch Set 3 : manifest tweek #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Messages
Total messages: 33 (12 generated)
Description was changed from ========== Enable the MojoDOMSTorageBrowserTests now that crbug/xxx is fixed. BUG= ========== to ========== Enable the MojoDOMSTorageBrowserTests now that crbug/608121 is fixed. BUG=586194,608121 ==========
michaeln@chromium.org changed reviewers: + rockot@chromium.org
Arghhhh... something else is still not quite right with osx. see patchset #1 for
mac test failures. Looks like its crashing after the test completes? Good news
is that seems to be working elsewhere.
Here's the log from the failed osx run. Looks like the test actually succeeds
and then the process crashes
[ RUN ] MojoDOMStorageBrowserTest.SanityCheckIncognito
[70193:1287:0504/120834:32167210379696:WARNING:vt_video_decode_accelerator_mac.cc(161)]
Failed to create VTDecompressionSession: Error Domain=NSOSStatusErrorDomain
Code=-8973 "The operation couldn’t be completed. (OSStatus error -8973.)"
(codecOpenErr) (-8973)
[70193:1287:0504/120834:32167211235256:WARNING:vt_video_decode_accelerator_mac.cc(196)]
Failed to create hardware VideoToolbox session
[70193:1287:0504/120834:32167266362720:ERROR:vt_video_encode_accelerator_mac.cc(507)]
VTCompressionSessionCreate failed: -12908
[70190:70151:0504/120834:32167456605640:ERROR:native_application_support.cc(46)]
Failed to load app library (path:
/b/swarm_slave/work/isolated/runMWc16f/out/Release/Mojo
Applications/tracing/tracing.mojo)
[371/1383] MojoDOMStorageBrowserTest.SanityCheckIncognito (1584 ms)
2 tests failed:
MojoDOMStorageBrowserTest.SanityCheck
(../../content/browser/dom_storage/dom_storage_browsertest.cc:60)
MojoDOMStorageBrowserTest.SanityCheckIncognito
(../../content/browser/dom_storage/dom_storage_browsertest.cc:64)
I'll update the CL to disable it on OSX and open another bug.
Description was changed from ========== Enable the MojoDOMSTorageBrowserTests now that crbug/608121 is fixed. BUG=586194,608121 ========== to ========== Enable the MojoDOMSTorageBrowserTests now that crbug/608121 is fixed. BUG=586194,609632 ==========
Description was changed from ========== Enable the MojoDOMSTorageBrowserTests now that crbug/608121 is fixed. BUG=586194,609632 ========== to ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 ==========
ptal, disabled on mac and opened 608121 about the osx specific problem.
lgtm
Drat, what's a capability spec? [5672:1068:0505/164002:65900453:ERROR:interface_registry.cc(41)] CapabilitySpec of: exe:content_browser prevented binding interface: leveldb::LevelDBService provided by: mojo:user
Oops. Need to modify content/public/app/content_browser_manifest.json and
add a new entry to the "required" block (as a sibling of the "mojo:shell"
entry):
"mojo:user": {
"interfaces": [ "leveldb::LevelDBService" ]
}
On May 5, 2016 5:13 PM, <michaeln@chromium.org> wrote:
> Drat, what's a capability spec?
>
> [5672:1068:0505/164002:65900453:ERROR:interface_registry.cc(41)]
> CapabilitySpec of: exe:content_browser prevented binding interface:
> leveldb::LevelDBService provided by: mojo:user
>
>
>
> https://codereview.chromium.org/1947213002/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
That helped to get a little farther, but now its crashing becuase |this|
instance has been deleted.
// Callback when NativeRunner completes.
void OnRunnerCompleted() {
if (!runner_.get())
return; // We're in the destructor.
shell_->OnInstanceError(this);
}
void Shell::OnInstanceError(Instance* instance) {
const Identity identity = instance->identity(); // CRASH
// Remove the shell.
auto it = identity_to_instance_.find(identity);
DCHECK(it != identity_to_instance_.end());
int id = instance->id();
identity_to_instance_.erase(it);
instance_listeners_.ForAllPtrs([this, id](mojom::InstanceListener* listener) {
listener->InstanceDestroyed(id);
});
delete instance;
if (!instance_quit_callback_.is_null())
instance_quit_callback_.Run(identity);
}
The instance gets deleted via a closure that calls...
void OnShellClientLost(base::WeakPtr<shell::Shell> shell) {
shell_client_.reset();
OnConnectionLost(shell);
}
... then a little later a different closure invokes
deletedobj->OnRunnerCompleted().
linix asan rel noticed the UAF
Xlib: extension "RANDR" missing on display ":9".
[25973:26051:0505/193817:67702684731:ERROR:native_application_support.cc(46)]
Failed to load app library (path: /tmp/runN7UzVy/out/Release/Mojo
Applications/tracing/tracing.mojo)
=================================================================
==25973==ERROR: AddressSanitizer: heap-use-after-free on address 0x6140000470f8
at pc 0x00000c4c015c bp 0x7fffa6d7d7e0 sp 0x7fffa6d7d7d8
READ of size 8 at 0x6140000470f8 thread T0 (content_browser)
#0 0xc4c015b in get
buildtools/third_party/libc++/trunk/include/memory:2714:69
#1 0xc4c015b in shell::Shell::Instance::OnRunnerCompleted()
services/shell/shell.cc:434
#2 0x229b521 in Run base/callback.h:397:12
#3 0x229b521 in base::debug::TaskAnnotator::RunTask(char const*,
base::PendingTask const&) base/debug/task_annotator.cc:51
#4 0x215df11 in base::MessageLoop::RunTask(base::PendingTask const&)
base/message_loop/message_loop.cc:484:3
#5 0x215ea75 in base::MessageLoop::DeferOrRunPendingTask(base::PendingTask
const&) base/message_loop/message_loop.cc:493:5
#6 0x215f94c in base::MessageLoop::DoWork()
base/message_loop/message_loop.cc:610:13
#7 0x2296806 in HandleDispatch base/message_loop/message_pump_glib.cc:267:7
#8 0x2296806 in base::(anonymous namespace)::WorkSourceDispatch(_GSource*,
int (*)(void*), void*) base/message_loop/message_pump_glib.cc:109
#9 0x7efedb5d3d12 in g_main_dispatch
/build/buildd/glib2.0-2.32.4/./glib/gmain.c:2539
#10 0x7efedb5d3d12 in g_main_context_dispatch
/build/buildd/glib2.0-2.32.4/./glib/gmain.c:3075
0x6140000470f8 is located 184 bytes inside of 432-byte region
[0x614000047040,0x6140000471f0)
freed by thread T0 (content_browser) here:
#0 0x57a0fb in operator delete(void*)
(/tmp/runN7UzVy/out/Release/content_browsertests+0x57a0fb)
#1 0xc4a65ee in shell::Shell::OnInstanceError(shell::Shell::Instance*)
services/shell/shell.cc:588:3
#2 0xc4b2bde in OnConnectionLost services/shell/shell.cc:419:7
#3 0xc4b2bde in
shell::Shell::Instance::OnShellClientLost(base::WeakPtr<shell::Shell>)
services/shell/shell.cc:411
#4 0xc4b3ffd in Run<const base::WeakPtr<shell::Shell> &>
base/bind_internal.h:181:12
#5 0xc4b3ffd in MakeItSo<shell::Shell::Instance *, const
base::WeakPtr<shell::Shell> &> base/bind_internal.h:321
#6 0xc4b3ffd in base::internal::Invoker<base::IndexSequence<0ul, 1ul>,
base::internal::BindState<base::internal::RunnableAdapter<void
(shell::Shell::Instance::*)(base::WeakPtr<shell::Shell>)>, void
(shell::Shell::Instance*, base::WeakPtr<shell::Shell>),
base::internal::UnretainedWrapper<shell::Shell::Instance>,
base::WeakPtr<shell::Shell> >, base::internal::InvokeHelper<false, void,
base::internal::RunnableAdapter<void
(shell::Shell::Instance::*)(base::WeakPtr<shell::Shell>)> >, void
()>::Run(base::internal::BindStateBase*) base/bind_internal.h:372
#7 0x1a31f58 in Run mojo/public/cpp/bindings/callback.h:82:7
#8 0x1a31f58 in mojo::internal::Router::OnConnectionError()
mojo/public/cpp/bindings/lib/router.cc:312
#9 0x1a06e8c in Run mojo/public/cpp/bindings/callback.h:82:7
#10 0x1a06e8c in mojo::internal::Connector::HandleError(bool, bool)
mojo/public/cpp/bindings/lib/connector.cc:342
previously allocated by thread T0 (content_browser) here:
#0 0x579b3b in operator new(unsigned long)
(/tmp/runN7UzVy/out/Release/content_browsertests+0x579b3b)
#1 0xc4a4168 in shell::Shell::CreateInstance(shell::Identity const&,
shell::Identity const&, shell::CapabilitySpec const&)
services/shell/shell.cc:655:24
#2 0xc4ab3ac in
shell::Shell::OnGotResolvedName(std::__1::unique_ptr<shell::ConnectParams,
std::__1::default_delete<shell::ConnectParams> >,
mojo::InterfacePtr<shell::mojom::ShellClient>,
mojo::StructPtr<shell::mojom::ResolveResult>) services/shell/shell.cc:753:24
ptal at the fix for the Shell/InProcessNativeRunner crash https://codereview.chromium.org/1947213002/diff/60001/services/shell/shell.cc File services/shell/shell.cc (left): https://codereview.chromium.org/1947213002/diff/60001/services/shell/shell.cc... services/shell/shell.cc:219: // We own |runner_|, so Unretained is safe here. The InProcessNativeRunner wraps the OnRunnerComplete callback like this.. DCHECK(app_completed_callback_runner_.is_null()); app_completed_callback_runner_ = base::Bind( &base::TaskRunner::PostTask, base::ThreadTaskRunnerHandle::Get(), FROM_HERE, app_completed_callback); so there's no guarantee |this| instance is not deleted at the time the callback is run. Idk if calling thru a weakptr is the best option but it does avoid the crash. There are other Unretained instance ptrs in this class, i could switch them all over to the weakptr if you like. Alternatively, I could change InProcessNativeRunner to not call the RunnerComplete callback after it has been deleted.
Ah hrmm... So the fix to InProecessNativeRunner looks good and I agree it's necessary to use WeakPtr there when Binding. It is not necessary to use it elsewhere (for say, mojo callbacks) so please don't update other bind callsites. This LGTM if the test runs and passes but there's one major problem here: InProcessNativeRunner shouldn't be used at all. What exactly is it running?
It's trying to run the tracing app, which i think is intentionally not packaged
up and to not load. I don't know what's trying to load that or why?
Regarding whether if fixed the crash... yes, but the beat goes on :(
==13376==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 16 byte(s) in 1 object(s) allocated from:
#0 0x579c7b in operator new(unsigned long)
(/tmp/runQeVaKk/out/Release/content_browsertests+0x579c7b)
#1 0xc4c971d in AddInterface<user_service::mojom::UserService>
services/shell/public/cpp/interface_registry.h:77:9
#2 0xc4c971d in AddInterface<user_service::mojom::UserService>
services/shell/public/cpp/connection.h:74
#3 0xc4c971d in
user_service::UserShellClient::AcceptConnection(shell::Connection*)
services/user/user_shell_client.cc:104
#4 0xcc70463 in
shell::ShellConnection::AcceptConnection(mojo::InlinedStructPtr<shell::mojom::Identity>,
unsigned int, mojo::InterfaceRequest<shell::mojom::InterfaceProvider>,
mojo::InterfacePtr<shell::mojom::InterfaceProvider>,
mojo::StructPtr<shell::mojom::CapabilityRequest>, mojo::String const&)
services/shell/public/cpp/lib/shell_connection.cc:82:8
#5 0xccab448 in shell::mojom::ShellClientStub::Accept(mojo::Message*)
/b/build/slave/linux_asan/build/src/out/Release/gen/services/shell/public/interfaces/shell_client.mojom.cc:585:7
#6 0x1a2a402 in
mojo::internal::Router::HandleMessageInternal(mojo::Message*)
mojo/public/cpp/bindings/lib/router.cc:287:12
#7 0x1a27d89 in
mojo::internal::Router::HandleIncomingMessage(mojo::Message*)
mojo/public/cpp/bindings/lib/router.cc:217:10
#8 0xccacdcf in
shell::mojom::ShellClientRequestValidator::Accept(mojo::Message*)
/b/build/slave/linux_asan/bui
The CQ bit was checked by michaeln@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org Link to the patchset: https://codereview.chromium.org/1947213002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947213002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
rockot@chromium.org changed reviewers: + jam@chromium.org
Please add an OWNERS file to content/public/app/mojo with myself and ben@ in it. +jam to review that.
On 2016/05/09 at 18:28:50, Ken Rockot wrote: > Please add an OWNERS file to content/public/app/mojo with myself and ben@ in it. +jam to review that. (or i guess i could add that in a separate CL...)
+jam for owners
Description was changed from ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 ========== to ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 TBR=jam ==========
The CQ bit was checked by michaeln@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1947213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1947213002/80001
FYI 609632 is fixed on ToT, so you could land this without disabling them on OS X.
Message was sent while issue was closed.
Description was changed from ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 TBR=jam ========== to ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 TBR=jam ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 TBR=jam ========== to ========== Enable the MojoDOMSTorageBrowserTests on most platforms now that crbug/608121 is fixed. Still disabled on osx until 609632 is figured out. BUG=586194,609632,608121 TBR=jam Committed: https://crrev.com/6936a1c58d788c0f6ff847a139c0ff3918aa65b4 Cr-Commit-Position: refs/heads/master@{#392393} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6936a1c58d788c0f6ff847a139c0ff3918aa65b4 Cr-Commit-Position: refs/heads/master@{#392393}
Message was sent while issue was closed.
i'll do that next |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
