Crash about dom storage context while browsing Twitter in private tabs with Tor #14392

riastradh-brave · 2018-06-12T17:05:14Z

Description

When in a private tab with Tor enabled, clicking on a link summary embedded in a tweet in an expanded Twitter thread in a new tab causes the browser to crash.

Steps to Reproduce

Enable Tor for private tabs.
In a private tab, open a Twitter feed, e.g. https://twitter.com/nytimes.
Control-click on a tweet with an embedded link summary to expand the thread in a new tab.
Switch to the new tab.
Click on the link summary.

Please also see the following for other reproducible cases:

Crash about dom storage context while browsing Twitter in private tabs with Tor #14392 (comment)
Crash about dom storage context while browsing Twitter in private tabs with Tor #14392 (comment)

Actual result:
The browser crashes with the following stack trace.

[18120:18120:0612/164743.717397:FATAL:web_contents_impl.cc(2294)] Check failed: session_storage_namespace_impl->IsFromContext(dom_storage_context). 
#0 0x00000314badc base::debug::StackTrace::StackTrace()
#1 0x000003163743 logging::LogMessage::~LogMessage()
#2 0x000002c0d9cd content::WebContentsImpl::CreateNewWindow()
#3 0x0000029ce794 content::RenderFrameHostImpl::CreateNewWindow()
#4 0x0000022cfdec content::mojom::FrameHostStubDispatch::AcceptWithResponder()
#5 0x0000029d8456 content::mojom::FrameHostStub<>::AcceptWithResponder()
#6 0x000003757f88 mojo::InterfaceEndpointClient::HandleValidatedMessage()
#7 0x00000374f354 IPC::(anonymous namespace)::ChannelAssociatedGroupController::AcceptSyncMessage()
#8 0x00000314ce6d base::debug::TaskAnnotator::RunTask()
#9 0x000003166c77 base::MessageLoop::RunTask()
#10 0x000003167147 base::MessageLoop::DoWork()
#11 0x0000031696af base::(anonymous namespace)::WorkSourceDispatch()
#12 0x7fba4115ffb7 g_main_context_dispatch
#13 0x7fba411601f0 <unknown>
#14 0x7fba4116027c g_main_context_iteration
#15 0x000003169572 base::MessagePumpGlib::Run()
#16 0x000003185c35 base::RunLoop::Run()
#17 0x0000028ba581 content::BrowserMainLoop::MainMessageLoopRun()
#18 0x0000028ba383 content::BrowserMainLoop::RunMainMessageLoopParts()
#19 0x0000028bd112 content::BrowserMainRunnerImpl::Run()
#20 0x0000028b662c content::BrowserMain()
#21 0x0000027fadc8 content::ContentMainRunnerImpl::Run()
#22 0x00000448d815 service_manager::Main()
#23 0x0000026b58e4 content::ContentMain()
#24 0x0000017a53c0 main
#25 0x7fba3bd461c1 __libc_start_main
#26 0x0000017a502a _start

Expected result:
The browser follows the link and opens it.

Reproduces how often:
100%

Brave Version

about:brave info:

Brave	0.23.8
V8	6.6.346.32
rev	`06c657b`
Muon	6.0.11
OS Release	4.13.0-41-generic
Update Channel
OS Architecture	x64
OS Platform	Linux
Node.js	7.9.0
Brave Sync	v1.4.2
libchromiumcontent	66.0.3359.170

Reproducible on current live release:
no

Additional Information

This does not happen in non-private tabs. This does not happen in private tabs with Tor disabled.

I will neither confirm nor deny whether I discovered this while at work.

The text was updated successfully, but these errors were encountered:

riastradh-brave · 2018-06-12T17:24:58Z

@darkdh

This is necessary because we use `persist:tor' since for hysterical raisins there's only one normal `private' partition with in_memory_ = true. We use the virtual method IsOffTheRecord() to discriminate instead. Fixes #608. Fixes brave/browser-laptop#14392. Auditors: @darkdh

riastradh-brave · 2018-06-14T23:42:25Z

Fixed in brave/muon@96c2eb7.

@darkdh

This is necessary because we use `persist:tor' since for hysterical raisins there's only one normal `private' partition with in_memory_ = true. We use the virtual method IsOffTheRecord() to discriminate instead. Fixes #608. Fixes brave/browser-laptop#14392. Auditors: @darkdh

@darkdh

This is necessary because we use `persist:tor' since for hysterical raisins there's only one normal `private' partition with in_memory_ = true. We use the virtual method IsOffTheRecord() to discriminate instead. Fixes #608. Fixes brave/browser-laptop#14392. Auditors: @darkdh

@darkdh

This is necessary because we use `persist:tor' since for hysterical raisins there's only one normal `private' partition with in_memory_ = true. We use the virtual method IsOffTheRecord() to discriminate instead. Fixes #608. Fixes brave/browser-laptop#14392. Auditors: @darkdh

@darkdh

This is necessary because we use `persist:tor' since for hysterical raisins there's only one normal `private' partition with in_memory_ = true. We use the virtual method IsOffTheRecord() to discriminate instead. Fixes #608. Fixes brave/browser-laptop#14392. Auditors: @darkdh

riastradh-brave · 2018-07-06T17:18:36Z

Either something re-broke this or the earlier change I cited as a fix only incidentally papered over the problem. It is back in 0.23.19 and in master.

kjozwiak · 2018-07-13T03:23:11Z

@bsclifton should we add this into a milestone? Maybe release 4? Crashing is never a good thing, especially with a new feature like Tor.. Thoughts?

jumde · 2018-07-13T04:34:58Z

@riastradh-brave @kjozwiak can't repro on 0.23.31

bsclifton · 2018-07-13T05:54:30Z

@kjozwiak sounds good- pulled into release 4

riastradh-brave · 2018-07-13T17:12:52Z

Still reproduces for me on 0.23.31.

darkdh · 2018-07-13T19:39:42Z

https://jsfiddle.net/dqokhmsg/
crash because of rel="noopener"

2357   scoped_refptr<SiteInstance> site_instance =
2358       // params.opener_suppressed && !is_guest
2359          params.opener_suppressed
2360           ? SiteInstance::CreateForURL(GetBrowserContext(), params.target_url)
2361           : source_site_instance;

so BraveContentBrowserClient::GetStoragePartitionConfigForSite for tor will get different partition_domain and partition_name

@bridiver

…er_suppressed(noopener) specified because WebContentsImpl::CreateNewWindow will use target_url as new site instance The problem was cloning original site instance cause the inconsistency between original partition and target partition because tor browser context enforce isolation storage so every different site has its own storage partition fix brave/browser-laptop#14392 Test Plan: 1. Open tor tab 2. Visit site contains rel="noopener" href (https://jsfiddle.net/dqokhmsg/) 3. Click the link 4. Brave shouldn't crash Auditors: @bridiver, @bbondy

kjozwiak · 2018-07-18T00:40:36Z

Another case @jasonrsadler mentioned in slack that @riastradh-brave confirmed was the same issues:

STR:

launch 0.23.31 3148ace
open Tor PT and load https://community.plus.net/t5/Tech-Help-Software-Hardware-etc/Which-computer-am-I-using/td-p/1061141
click on http://www.whatsmyip.org/more-info-about-you/ in the second reply in the thread

darkdh · 2018-07-18T00:41:52Z

Test Plan:

Open tor tab
Visit site contains rel="noopener" href (https://jsfiddle.net/dqokhmsg/)
Click the link
Brave shouldn't crash

srirambv · 2018-07-23T14:17:35Z

Verified on Windows 10 x64 using

0.23.70 e63c780
Muon 8.0.1
libchromiumcontent 68.0.3440.68

Verified on Ubuntu 17.10 x64

0.23.70 e63c780
Muon 8.0.1
libchromiumcontent 68.0.3440.68
verified browser doesn't crash on all 3 scenarios mentioned in the original issue

Verified with macOS 10.12.6 using

0.23.72 c3b1cac
Muon 8.0.2
libchromiumcontent 68.0.3440.75
verified browser doesn't crash on all 3 scenarios mentioned in the original issue

LaurenWags · 2018-07-23T14:47:24Z

Using 0.23.70 with muon 8.0.1 and STR from description still produces a crash on macOS for me. However, the steps from #14392 (comment) and #14392 (comment) do not crash. Requested @kjozwiak to give it a try on macOS as well. @srirambv could not reproduce the crash on Win.

btlechowski · 2018-07-23T15:50:14Z

The original STR still lead to crash, but I believe this is a different crash(#14806).

Tested 0.23.70 on Ubuntu 17.10

srirambv · 2018-07-23T15:55:12Z

Need to verify this(original steps) again once #14806 is fixed.

@darkdh

This is necessary because we use `persist:tor' since for hysterical raisins there's only one normal `private' partition with in_memory_ = true. We use the virtual method IsOffTheRecord() to discriminate instead. Fixes #608. Fixes brave/browser-laptop#14392. Auditors: @darkdh

@bridiver

…er_suppressed(noopener) specified because WebContentsImpl::CreateNewWindow will use target_url as new site instance The problem was cloning original site instance cause the inconsistency between original partition and target partition because tor browser context enforce isolation storage so every different site has its own storage partition fix brave/browser-laptop#14392 Test Plan: 1. Open tor tab 2. Visit site contains rel="noopener" href (https://jsfiddle.net/dqokhmsg/) 3. Click the link 4. Brave shouldn't crash Auditors: @bridiver, @bbondy

kjozwiak · 2018-07-27T04:49:38Z

Cleared QA/checked-Linux & QA/checked-Win64 so they can be rechecked as per #14392 (comment) now that #14806 has been fixed and verified.

GeetaSarvadnya · 2018-07-30T05:53:27Z

Verified on Windows x64 with
• 0.23.73 50bdb6d
• Muon 8.0.3
• libchromiumcontent 68.0.3440.75

Verified on Ubuntu 17.10 x64

0.23.73 50bdb6d
Muon 8.0.3
libchromiumcontent 68.0.3440.75

riastradh-brave added the feature/tor label Jun 12, 2018

jumde self-assigned this Jun 13, 2018

riastradh-brave mentioned this issue Jun 14, 2018

Propagate off-the-recordness to URL request context. brave/muon#609

Merged

riastradh-brave closed this as completed Jun 14, 2018

riastradh-brave reopened this Jul 6, 2018

riastradh-brave assigned darkdh Jul 6, 2018

bsclifton added this to the 0.23.x Release 4 milestone Jul 13, 2018

jumde removed their assignment Jul 13, 2018

darkdh mentioned this issue Jul 14, 2018

Use new site instance for SessionStorageNamespaceImpl clone when opener_suppressed(noopener) specified brave/muon#632

Merged

riastradh-brave added PR/pending-review and removed PR/pending-review labels Jul 17, 2018

alexwykoff modified the milestones: 0.23.x Release 4, 0.23.x w/ Chromium 68 (BETA Channel) Jul 17, 2018

alexwykoff added the release/not-blocking label Jul 17, 2018

kjozwiak added QA/test-plan-specified release-notes/include crash labels Jul 18, 2018

darkdh closed this as completed in brave/muon@756c3ca Jul 18, 2018

srirambv added the QA/checked-Win64 label Jul 23, 2018

btlechowski added the QA/checked-Linux label Jul 23, 2018

LaurenWags mentioned this issue Jul 25, 2018

Manual test run on OS X for 0.23.x Release 4 w/ Chromium 68 (BETA Channel) #14838

Closed

LaurenWags added the QA/checked-macOS label Jul 25, 2018

kjozwiak removed QA/checked-Linux QA/checked-Win64 labels Jul 27, 2018

This was referenced Jul 27, 2018

Manual test run on Windows x64 for 0.23.x Release 4 w/ Chromium 68 (BETA Channel) #14859

Closed

Manual test run on Linux for 0.23.x Release 4 w/ Chromium 68 (BETA Channel) #14860

Closed

GeetaSarvadnya added the QA/checked-Win64 label Jul 30, 2018

btlechowski added the QA/checked-Linux label Jul 30, 2018

kjozwiak mentioned this issue Jul 31, 2018

Release notes for 0.23.x Release 4 /w Chromium 68 #14884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Crash about dom storage context while browsing Twitter in private tabs with Tor #14392

Crash about dom storage context while browsing Twitter in private tabs with Tor #14392

riastradh-brave commented Jun 12, 2018 •

edited by kjozwiak

Loading

riastradh-brave commented Jun 12, 2018

riastradh-brave commented Jun 14, 2018

riastradh-brave commented Jul 6, 2018

kjozwiak commented Jul 13, 2018

jumde commented Jul 13, 2018

bsclifton commented Jul 13, 2018

riastradh-brave commented Jul 13, 2018

darkdh commented Jul 13, 2018 •

edited

Loading

kjozwiak commented Jul 18, 2018 •

edited

Loading

darkdh commented Jul 18, 2018

srirambv commented Jul 23, 2018 •

edited by LaurenWags

Loading

LaurenWags commented Jul 23, 2018

btlechowski commented Jul 23, 2018

srirambv commented Jul 23, 2018

kjozwiak commented Jul 27, 2018

GeetaSarvadnya commented Jul 30, 2018 •

edited by btlechowski

Loading

Crash about dom storage context while browsing Twitter in private tabs with Tor #14392

Crash about dom storage context while browsing Twitter in private tabs with Tor #14392

Comments

riastradh-brave commented Jun 12, 2018 • edited by kjozwiak Loading

Description

Steps to Reproduce

Brave Version

Additional Information

riastradh-brave commented Jun 12, 2018

riastradh-brave commented Jun 14, 2018

riastradh-brave commented Jul 6, 2018

kjozwiak commented Jul 13, 2018

jumde commented Jul 13, 2018

bsclifton commented Jul 13, 2018

riastradh-brave commented Jul 13, 2018

darkdh commented Jul 13, 2018 • edited Loading

kjozwiak commented Jul 18, 2018 • edited Loading

darkdh commented Jul 18, 2018

srirambv commented Jul 23, 2018 • edited by LaurenWags Loading

LaurenWags commented Jul 23, 2018

btlechowski commented Jul 23, 2018

srirambv commented Jul 23, 2018

kjozwiak commented Jul 27, 2018

GeetaSarvadnya commented Jul 30, 2018 • edited by btlechowski Loading

riastradh-brave commented Jun 12, 2018 •

edited by kjozwiak

Loading

darkdh commented Jul 13, 2018 •

edited

Loading

kjozwiak commented Jul 18, 2018 •

edited

Loading

srirambv commented Jul 23, 2018 •

edited by LaurenWags

Loading

GeetaSarvadnya commented Jul 30, 2018 •

edited by btlechowski

Loading