Chromium Code Reviews Chromium Code Reviews
Issue
1770763003:
Revert of Partial implementation of subscription restrictions. (Closed)
DescriptionRevert of Partial implementation of subscription restrictions. (patchset #8 id:140001 of https://codereview.chromium.org/1701313002/ )
Reason for revert:
Sorry about the delay in reverting ...
This is causing many memory errors on DrMemory.
I think there is an error in the patch, in push_messaging_dispatcher.cc. The same WebPush...Callbacks* can be added twice to |subscription_callbacks_|, in the new DidGetManifest and in the already existing DoSubscribe.
As |subscription_callbacks_| is declared with IDMapOwnPointer, this will cause callbacks to be freed twice.
First build where it failed with example errors:
https://build.chromium.org/p/chromium.memory.fyi/builders/Windows%20Browser%20%28DrMemory%20full%29%20%281%29/builds/2799
Sample output:
UNADDRESSABLE ACCESS beyond heap bounds: writing 0x09880330-0x09880334 4 byte(s)
# 0 modules.dll!blink::WebCallbacks<>::`scalar deleting destructor'
# 1 content.dll!IDMap<>::Releaser<>::release_all [base\id_map.h:250]
# 2 content.dll!content::PushMessagingDispatcher::~PushMessagingDispatcher [content\renderer\push_messaging\push_messaging_dispatcher.cc:27]
# 3 content.dll!content::PushMessagingDispatcher::`scalar deleting destructor'
# 4 content.dll!content::RenderFrameObserver::OnDestruct [content\public\renderer\render_frame_observer.cc:33]
# 5 content.dll!content::RenderFrameImpl::`vector deleting destructor'
# 6 content.dll!content::RenderFrameImpl::frameDetached [content\renderer\render_frame_impl.cc:2740]
# 7 blink_web.dll!blink::FrameLoaderClientImpl::detached [third_party\webkit\source\web\frameloaderclientimpl.cpp:383]
# 8 webcore_shared.dll!blink::Frame::detach [third_party\webkit\source\core\frame\frame.cpp:98]
# 9 webcore_shared.dll!blink::LocalFrame::detach [third_party\webkit\source\core\frame\localframe.cpp:357]
#10 webcore_shared.dll!blink::Page::willBeDestroyed [third_party\webkit\source\core\page\page.cpp:573]
#11 base.dll!base::debug::TaskAnnotator::RunTask [base\debug\task_annotator.cc:51]
#12 scheduler.dll!scheduler::TaskQueueManager::ProcessTaskFromWorkQueue [components\scheduler\base\task_queue_manager.cc:288]
#13 scheduler.dll!scheduler::TaskQueueManager::DoWork [components\scheduler\base\task_queue_manager.cc:200]
#14 scheduler.dll!base::internal::Invoker<>::Run [base\bind_internal.h:354]
#15 base.dll!base::debug::TaskAnnotator::RunTask [base\debug\task_annotator.cc:51]
#16 base.dll!base::MessageLoop::RunTask [base\message_loop\message_loop.cc:476]
#17 base.dll!base::MessageLoop::DeferOrRunPendingTask [base\message_loop\message_loop.cc:485]
#18 base.dll!base::MessageLoop::DoWork [base\message_loop\message_loop.cc:597]
#19 base.dll!base::MessagePumpDefault::Run [base\message_loop\message_pump_default.cc:33]
#20 base.dll!base::MessageLoop::RunHandler [base\message_loop\message_loop.cc:440]
#21 base.dll!base::RunLoop::Run [base\run_loop.cc:35]
#22 base.dll!base::MessageLoop::Run [base\message_loop\message_loop.cc:293]
#23 content.dll!content::RendererMain [content\renderer\renderer_main.cc:219]
#24 content.dll!content::RunNamedProcessTypeMain [content\app\content_main_runner.cc:395]
#25 content.dll!content::ContentMainRunnerImpl::Run [content\app\content_main_runner.cc:766]
#26 content.dll!content::ContentMain [content\app\content_main.cc:19]
#27 content::LaunchTests [content\public\test\test_launcher.cc:505]
#28 LaunchChromeTests [chrome\test\base\chrome_test_launcher.cc:128]
#29 main [chrome\test\base\browser_tests_main.cc:21]
Note: @0:03:41.960 in thread 1944
Note: next higher malloc: 0x098804a0-0x098804c0
Note: prev lower malloc: 0x09880248-0x09880310
Note: instruction: mov $0x679f7ef4 -> (%esi)
The report came from the `PushMessagingBrowserTest.GrantAlreadyGrantedPermissionDoesNotUnsubscribe` test.
Original issue's description:
> Partial implementation of subscription restrictions.
>
> Currently, chrome requires that app developers provide a "sender_id" tag in the
> manifest. This id is generated by the developer console and is not standard
> for all browsers.
>
> In the future, the app developer will be able to specify a public key for their
> service, which will be registered with the push service and which the push
> service can use to validate app services requesting to send messages.
>
> BUG=583753
>
> Committed: https://crrev.com/71f5a08bb30153b56e8a3f9b447264a54b6d9c12
> Cr-Commit-Position: refs/heads/master@{#379045}
TBR=mkwst@chromium.org,avi@chromium.org,mvanouwerkerk@chromium.org,peter@chromium.org,harkness@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=583753
Committed: https://crrev.com/cd6d01e60cd874019473ae1c37eb5227b5ed6daf
Cr-Commit-Position: refs/heads/master@{#379750}
Patch Set 1 #Messages
Total messages: 17 (2 generated)
|
