Use frame_tree_node_id to get web-contents in case of no process ID #5
+29
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bridiver
reviewed
Dec 19, 2017
| @@ -41,19 +41,25 @@ void BraveShieldsWebContentsObserver::RenderFrameCreated( | |||
| void BraveShieldsWebContentsObserver::DispatchBlockedEvent( | |||
| const std::string& block_type, | |||
| int render_process_id, | |||
| int render_frame_id) { | |||
| int render_frame_id, | |||
| int frame_tree_node_id) { | |||
| DCHECK_CURRENTLY_ON(content::BrowserThread::UI); | |||
| if (render_frame_id == -1 || render_process_id == -1) { | |||
There was a problem hiding this comment.
this check needs to be removed
bbondy
force-pushed
the
plz-navigae-c63
branch
from
44dd6c7
to
aa8e03f
Compare
bbondy
force-pushed
the
plz-navigae-c63
branch
from
aa8e03f
to
5ed02f1
Compare
bridiver
approved these changes
Dec 19, 2017
NejcZdovc
pushed a commit
that referenced
this pull request
Dec 10, 2018
bug fix: passing references to locals to PostTask
bbondy
added a commit
that referenced
this pull request
Feb 18, 2019
Add HTTPS Everywhere and JS blocking support
mkarolin
added a commit
that referenced
this pull request
Sep 25, 2019
Chromium changes: https://chromium.googlesource.com/chromium/src/+/26e73cfd90affbf1bdf3efbe80bf2903b0e866ae commit 26e73cfd90affbf1bdf3efbe80bf2903b0e866ae Author: Yutaka Hirano <[email protected]> Date: Tue Aug 6 02:38:13 2019 +0000 Stop giving WebSocketClient to WebSocket creation function Instead, give it as an argument of mojom.WebSocketHandshakeClient.OnConnectionEstablished. This is a follow up CL for https://chromium-review.googlesource.com/c/chromium/src/+/1728917. Passing the mojo::InterfaceRequest<WebSocketClient> at OnConnectionEstablished is less error prone because 1) It is unable to bind the implementation before that, and 2) It is now clear that we should detect connection errors on |handshake_client| until the connection is established, and |client| after that. Bug: 989406, 967524 https://chromium.googlesource.com/chromium/src/+/1007403a797957b8af46e962cdfc1f6f671954a4 commit 1007403a797957b8af46e962cdfc1f6f671954a4 Author: Yoichi Osato <[email protected]> Date: Thu Aug 8 13:11:57 2019 +0000 [WebSocket] Use Datapipe to transfer DataFrame buffer instead of ReadOnlyBuffer This CL introduces mojo DataPipe instead of ReadOnlyBuffer. With DataPipe, we can reduce memory copy and optimize quota control. Unfortunately, this CL makes performance slower: ToT: 147 MB/s Patch: 128 MB/s That's because this CL replaces minimize part and we will do the optimization later on. If the optimization doesn't go over the original score before M78 (will be branch cut on Sep 5th), we will revert this change. Bug: 865001 https://chromium.googlesource.com/chromium/src/+/fcaa2a2c441e28a5cb95cc90946712efb0ce085f commit fcaa2a2c441e28a5cb95cc90946712efb0ce085f Author: Yoichi Osato <[email protected]> Date: Wed Aug 28 08:22:36 2019 +0000 [WebSocket] Remove manual quota control for receiving. Because we're using mojo data pipe, which has own quota control, to transfer received data, we don't have manual one, or mojom::WebSocket.AddReceiveFlowControlQuota(). This CL does a sort of work removing the function. mojom::WebSocket.AddReceiveFlowControlQuota(quota) did 5 tasks: browser side(mainly on WebSocketChannel) 1. Trigger the first ReadFrame if renderer gets not throttled. For #1, this patch moves the trigger to mojo WebSocket.StartReceiving. 2. Trigger next ReadFrame if there is enough quota. 3. Send pending dataframes based on added quota. This patch moves task #2 and #3 to websocket.cc and datapipe itself. 4. Dropchannel if all pending frames are received by renderer when closed. Because we can care #4 w/o renderer ping back, this patch move RespondToClosingHandshake()to WebSocketChannel::ReadFrames(). renderer side(mainly on WebSocketChannelImpl) 5. Ping browser that backpressure is turned off. For task #5, this patch changes not to call the mojo but throttle datapipe reading with WebSocketHandleImpl::ConsumePendingDataFrames(). receive-arraybuffer-1MBx100.htmll?iteration=100 measurement on local build: ToT: 144 MB/s (stdev: 4.56 MB/s) Patch: 208 MB/s (stdev: 6.15 MB/s) (+44% to ToT, +41% to ReadOnlyBuffer) ReadOnlyBuffer: 147 MB/s https://chromium.googlesource.com/chromium/src/+/a461132e64f53f45997aaa4222e05cdf5d10ea56 commit a461132e64f53f45997aaa4222e05cdf5d10ea56 Author: Julie Jeongeun Kim <[email protected]> Date: Fri Aug 30 04:13:36 2019 +0000 Reland "Convert WebSocket to new Mojo types" This is a reland of 06a7200b370f38f39c97a4810d0931ab5596014e It updates WebSocketChannelImplTest::EstablishConnection with new Mojo types without InterfacePtr and MakeRequest. [email protected] Original change's description: > Convert WebSocket to new Mojo types > > This CL converts WebSocketPtr, to new Mojo types and > updates OnConnectionEstablished from websocket.mojom. > > Bug: 955171, 978694 > Change-Id: I82416b209e2380241b64ebd3d829dd8bde5247c7 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1773016 > Commit-Queue: Julie Kim <[email protected]> > Reviewed-by: John Abd-El-Malek <[email protected]> > Reviewed-by: Sam McNally <[email protected]> > Reviewed-by: Yutaka Hirano <[email protected]> > Reviewed-by: Ken Rockot <[email protected]> > Cr-Commit-Position: refs/heads/master@{#691574} Bug: 955171, 978694
mkarolin
added a commit
that referenced
this pull request
Oct 3, 2019
Chromium changes: https://chromium.googlesource.com/chromium/src/+/26e73cfd90affbf1bdf3efbe80bf2903b0e866ae commit 26e73cfd90affbf1bdf3efbe80bf2903b0e866ae Author: Yutaka Hirano <[email protected]> Date: Tue Aug 6 02:38:13 2019 +0000 Stop giving WebSocketClient to WebSocket creation function Instead, give it as an argument of mojom.WebSocketHandshakeClient.OnConnectionEstablished. This is a follow up CL for https://chromium-review.googlesource.com/c/chromium/src/+/1728917. Passing the mojo::InterfaceRequest<WebSocketClient> at OnConnectionEstablished is less error prone because 1) It is unable to bind the implementation before that, and 2) It is now clear that we should detect connection errors on |handshake_client| until the connection is established, and |client| after that. Bug: 989406, 967524 https://chromium.googlesource.com/chromium/src/+/1007403a797957b8af46e962cdfc1f6f671954a4 commit 1007403a797957b8af46e962cdfc1f6f671954a4 Author: Yoichi Osato <[email protected]> Date: Thu Aug 8 13:11:57 2019 +0000 [WebSocket] Use Datapipe to transfer DataFrame buffer instead of ReadOnlyBuffer This CL introduces mojo DataPipe instead of ReadOnlyBuffer. With DataPipe, we can reduce memory copy and optimize quota control. Unfortunately, this CL makes performance slower: ToT: 147 MB/s Patch: 128 MB/s That's because this CL replaces minimize part and we will do the optimization later on. If the optimization doesn't go over the original score before M78 (will be branch cut on Sep 5th), we will revert this change. Bug: 865001 https://chromium.googlesource.com/chromium/src/+/fcaa2a2c441e28a5cb95cc90946712efb0ce085f commit fcaa2a2c441e28a5cb95cc90946712efb0ce085f Author: Yoichi Osato <[email protected]> Date: Wed Aug 28 08:22:36 2019 +0000 [WebSocket] Remove manual quota control for receiving. Because we're using mojo data pipe, which has own quota control, to transfer received data, we don't have manual one, or mojom::WebSocket.AddReceiveFlowControlQuota(). This CL does a sort of work removing the function. mojom::WebSocket.AddReceiveFlowControlQuota(quota) did 5 tasks: browser side(mainly on WebSocketChannel) 1. Trigger the first ReadFrame if renderer gets not throttled. For #1, this patch moves the trigger to mojo WebSocket.StartReceiving. 2. Trigger next ReadFrame if there is enough quota. 3. Send pending dataframes based on added quota. This patch moves task #2 and #3 to websocket.cc and datapipe itself. 4. Dropchannel if all pending frames are received by renderer when closed. Because we can care #4 w/o renderer ping back, this patch move RespondToClosingHandshake()to WebSocketChannel::ReadFrames(). renderer side(mainly on WebSocketChannelImpl) 5. Ping browser that backpressure is turned off. For task #5, this patch changes not to call the mojo but throttle datapipe reading with WebSocketHandleImpl::ConsumePendingDataFrames(). receive-arraybuffer-1MBx100.htmll?iteration=100 measurement on local build: ToT: 144 MB/s (stdev: 4.56 MB/s) Patch: 208 MB/s (stdev: 6.15 MB/s) (+44% to ToT, +41% to ReadOnlyBuffer) ReadOnlyBuffer: 147 MB/s https://chromium.googlesource.com/chromium/src/+/a461132e64f53f45997aaa4222e05cdf5d10ea56 commit a461132e64f53f45997aaa4222e05cdf5d10ea56 Author: Julie Jeongeun Kim <[email protected]> Date: Fri Aug 30 04:13:36 2019 +0000 Reland "Convert WebSocket to new Mojo types" This is a reland of 06a7200b370f38f39c97a4810d0931ab5596014e It updates WebSocketChannelImplTest::EstablishConnection with new Mojo types without InterfacePtr and MakeRequest. [email protected] Original change's description: > Convert WebSocket to new Mojo types > > This CL converts WebSocketPtr, to new Mojo types and > updates OnConnectionEstablished from websocket.mojom. > > Bug: 955171, 978694 > Change-Id: I82416b209e2380241b64ebd3d829dd8bde5247c7 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1773016 > Commit-Queue: Julie Kim <[email protected]> > Reviewed-by: John Abd-El-Malek <[email protected]> > Reviewed-by: Sam McNally <[email protected]> > Reviewed-by: Yutaka Hirano <[email protected]> > Reviewed-by: Ken Rockot <[email protected]> > Cr-Commit-Position: refs/heads/master@{#691574} Bug: 955171, 978694
mkarolin
added a commit
that referenced
this pull request
Oct 3, 2019
Chromium changes: https://chromium.googlesource.com/chromium/src/+/26e73cfd90affbf1bdf3efbe80bf2903b0e866ae commit 26e73cfd90affbf1bdf3efbe80bf2903b0e866ae Author: Yutaka Hirano <[email protected]> Date: Tue Aug 6 02:38:13 2019 +0000 Stop giving WebSocketClient to WebSocket creation function Instead, give it as an argument of mojom.WebSocketHandshakeClient.OnConnectionEstablished. This is a follow up CL for https://chromium-review.googlesource.com/c/chromium/src/+/1728917. Passing the mojo::InterfaceRequest<WebSocketClient> at OnConnectionEstablished is less error prone because 1) It is unable to bind the implementation before that, and 2) It is now clear that we should detect connection errors on |handshake_client| until the connection is established, and |client| after that. Bug: 989406, 967524 https://chromium.googlesource.com/chromium/src/+/1007403a797957b8af46e962cdfc1f6f671954a4 commit 1007403a797957b8af46e962cdfc1f6f671954a4 Author: Yoichi Osato <[email protected]> Date: Thu Aug 8 13:11:57 2019 +0000 [WebSocket] Use Datapipe to transfer DataFrame buffer instead of ReadOnlyBuffer This CL introduces mojo DataPipe instead of ReadOnlyBuffer. With DataPipe, we can reduce memory copy and optimize quota control. Unfortunately, this CL makes performance slower: ToT: 147 MB/s Patch: 128 MB/s That's because this CL replaces minimize part and we will do the optimization later on. If the optimization doesn't go over the original score before M78 (will be branch cut on Sep 5th), we will revert this change. Bug: 865001 https://chromium.googlesource.com/chromium/src/+/fcaa2a2c441e28a5cb95cc90946712efb0ce085f commit fcaa2a2c441e28a5cb95cc90946712efb0ce085f Author: Yoichi Osato <[email protected]> Date: Wed Aug 28 08:22:36 2019 +0000 [WebSocket] Remove manual quota control for receiving. Because we're using mojo data pipe, which has own quota control, to transfer received data, we don't have manual one, or mojom::WebSocket.AddReceiveFlowControlQuota(). This CL does a sort of work removing the function. mojom::WebSocket.AddReceiveFlowControlQuota(quota) did 5 tasks: browser side(mainly on WebSocketChannel) 1. Trigger the first ReadFrame if renderer gets not throttled. For #1, this patch moves the trigger to mojo WebSocket.StartReceiving. 2. Trigger next ReadFrame if there is enough quota. 3. Send pending dataframes based on added quota. This patch moves task #2 and #3 to websocket.cc and datapipe itself. 4. Dropchannel if all pending frames are received by renderer when closed. Because we can care #4 w/o renderer ping back, this patch move RespondToClosingHandshake()to WebSocketChannel::ReadFrames(). renderer side(mainly on WebSocketChannelImpl) 5. Ping browser that backpressure is turned off. For task #5, this patch changes not to call the mojo but throttle datapipe reading with WebSocketHandleImpl::ConsumePendingDataFrames(). receive-arraybuffer-1MBx100.htmll?iteration=100 measurement on local build: ToT: 144 MB/s (stdev: 4.56 MB/s) Patch: 208 MB/s (stdev: 6.15 MB/s) (+44% to ToT, +41% to ReadOnlyBuffer) ReadOnlyBuffer: 147 MB/s https://chromium.googlesource.com/chromium/src/+/a461132e64f53f45997aaa4222e05cdf5d10ea56 commit a461132e64f53f45997aaa4222e05cdf5d10ea56 Author: Julie Jeongeun Kim <[email protected]> Date: Fri Aug 30 04:13:36 2019 +0000 Reland "Convert WebSocket to new Mojo types" This is a reland of 06a7200b370f38f39c97a4810d0931ab5596014e It updates WebSocketChannelImplTest::EstablishConnection with new Mojo types without InterfacePtr and MakeRequest. [email protected] Original change's description: > Convert WebSocket to new Mojo types > > This CL converts WebSocketPtr, to new Mojo types and > updates OnConnectionEstablished from websocket.mojom. > > Bug: 955171, 978694 > Change-Id: I82416b209e2380241b64ebd3d829dd8bde5247c7 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1773016 > Commit-Queue: Julie Kim <[email protected]> > Reviewed-by: John Abd-El-Malek <[email protected]> > Reviewed-by: Sam McNally <[email protected]> > Reviewed-by: Yutaka Hirano <[email protected]> > Reviewed-by: Ken Rockot <[email protected]> > Cr-Commit-Position: refs/heads/master@{#691574} Bug: 955171, 978694
mariospr
added a commit
that referenced
this pull request
Mar 11, 2022
This fixes several upstream browser tests crashing on DCHECK-enabled builds because of calls to dom_distiller::RunIsolatedJavaScript() being run without the ID being previously set, with erros like this: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is necessary or all browser tests will crash when running on CI.
mariospr
added a commit
that referenced
this pull request
Mar 11, 2022
BraveMainDelegate::BasicStartupComplete() is where several features are enabled and disabled for Brave via command line switches, but that method is not being called when running usptream tests, which use ChromeMainDelegate instead. Because of this, all browser tests crash on DCHECK-enabled builds with a backtrace like the following one: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is because the DOM Distiller feature is not enabled as expected (for Brave) on startup, and when AdsTabHelper::RunIsolatedJavaScript() (called via the WebContentsObserver machinery) calls dom_distiller::RunIsolatedJavaScript() it will fail the DCHECK that makes sure the JS World ID is already set. To fix this, we move the code of BraveMainDelegate::BasicStartupComplete() to ChromeMainDelegate::BasicStartupComplete() via a chromium_src override, so that we make sure that exactly the same initialization done for Brave and Brave-related browser tests also applies when running upstrem browser tests. This does not only fix that crash, but also makes sure that upstream browser tests are run with the same features as Brave, which is the right thing to do.
mariospr
added a commit
that referenced
this pull request
Mar 14, 2022
This fixes several upstream browser tests crashing on DCHECK-enabled builds because of calls to dom_distiller::RunIsolatedJavaScript() being run without the ID being previously set, with erros like this: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is necessary or all browser tests will crash when running on CI.
mariospr
added a commit
that referenced
this pull request
Mar 14, 2022
BraveMainDelegate::BasicStartupComplete() is where several features are enabled and disabled for Brave via command line switches, but that method is not being called when running usptream tests, which use ChromeMainDelegate instead. Because of this, all browser tests crash on DCHECK-enabled builds with a backtrace like the following one: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is because the DOM Distiller feature is not enabled as expected (for Brave) on startup, and when AdsTabHelper::RunIsolatedJavaScript() (called via the WebContentsObserver machinery) calls dom_distiller::RunIsolatedJavaScript() it will fail the DCHECK that makes sure the JS World ID is already set. To fix this, we move the code of BraveMainDelegate::BasicStartupComplete() to ChromeMainDelegate::BasicStartupComplete() via a chromium_src override, so that we make sure that exactly the same initialization done for Brave and Brave-related browser tests also applies when running upstrem browser tests. This does not only fix that crash, but also makes sure that upstream browser tests are run with the same features as Brave, which is the right thing to do.
mariospr
added a commit
that referenced
this pull request
Mar 15, 2022
This fixes several upstream browser tests crashing on DCHECK-enabled builds because of calls to dom_distiller::RunIsolatedJavaScript() being run without the ID being previously set, with erros like this: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is necessary or all browser tests will crash when running on CI.
mariospr
added a commit
that referenced
this pull request
Mar 15, 2022
BraveMainDelegate::BasicStartupComplete() is where several features are enabled and disabled for Brave via command line switches, but that method is not being called when running usptream tests, which use ChromeMainDelegate instead. Because of this, all browser tests crash on DCHECK-enabled builds with a backtrace like the following one: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is because the DOM Distiller feature is not enabled as expected (for Brave) on startup, and when AdsTabHelper::RunIsolatedJavaScript() (called via the WebContentsObserver machinery) calls dom_distiller::RunIsolatedJavaScript() it will fail the DCHECK that makes sure the JS World ID is already set. To fix this, we move the code of BraveMainDelegate::BasicStartupComplete() to ChromeMainDelegate::BasicStartupComplete() via a chromium_src override, so that we make sure that exactly the same initialization done for Brave and Brave-related browser tests also applies when running upstrem browser tests. This does not only fix that crash, but also makes sure that upstream browser tests are run with the same features as Brave, which is the right thing to do.
mariospr
added a commit
that referenced
this pull request
Mar 15, 2022
This fixes several upstream browser tests crashing on DCHECK-enabled builds because of calls to dom_distiller::RunIsolatedJavaScript() being run without the ID being previously set, with erros like this: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is necessary or all browser tests will crash when running on CI.
mariospr
added a commit
that referenced
this pull request
Mar 15, 2022
BraveMainDelegate::BasicStartupComplete() is where several features are enabled and disabled for Brave via command line switches, but that method is not being called when running usptream tests, which use ChromeMainDelegate instead. Because of this, all browser tests crash on DCHECK-enabled builds with a backtrace like the following one: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is because the DOM Distiller feature is not enabled as expected (for Brave) on startup, and when AdsTabHelper::RunIsolatedJavaScript() (called via the WebContentsObserver machinery) calls dom_distiller::RunIsolatedJavaScript() it will fail the DCHECK that makes sure the JS World ID is already set. To fix this, we move the code of BraveMainDelegate::BasicStartupComplete() to ChromeMainDelegate::BasicStartupComplete() via a chromium_src override, so that we make sure that exactly the same initialization done for Brave and Brave-related browser tests also applies when running upstrem browser tests. This does not only fix that crash, but also makes sure that upstream browser tests are run with the same features as Brave, which is the right thing to do.
mariospr
added a commit
that referenced
this pull request
Mar 23, 2022
BraveMainDelegate::BasicStartupComplete() is where several features are enabled and disabled for Brave via command line switches, but that method is not being called when running usptream tests, which use ChromeMainDelegate instead. Because of this, all browser tests crash on DCHECK-enabled builds with a backtrace like the following one: [497557:497557:1221/135319.265636:FATAL:distiller_javascript_utils.cc(41)] Check failed: distiller_javascript_world_id != invalid_world_id. #0 0x7fd8cc5b7459 base::debug::CollectStackTrace() #1 0x7fd8cc4b12d3 base::debug::StackTrace::StackTrace() #2 0x7fd8cc4d3a34 logging::LogMessage::~LogMessage() #3 0x7fd8cc4d44ae logging::LogMessage::~LogMessage() #4 0x56550ee1de70 dom_distiller::RunIsolatedJavaScript() #5 0x56550fffc990 brave_ads::AdsTabHelper::RunIsolatedJavaScript() #6 0x7fd8c6332b3d content::WebContentsImpl::WebContentsObserverList::NotifyObservers<>() #7 0x7fd8c63531a9 content::WebContentsImpl::DocumentOnLoadCompleted() [...] #56 0x56550ccba27a _start Task trace: #0 0x7fd8c6c6ebee IPC::(anonymous namespace)::ChannelAssociatedGroupController::Accept() #1 0x7fd8cbce69df mojo::SimpleWatcher::Context::Notify() Crash keys: "ui_scheduler_async_stack" = "0x7FD8C6C6EBEE 0x7FD8CBCE69DF" "io_scheduler_async_stack" = "0x7FD8CBCE69DF 0x0" [1/1] TabStripBrowsertest.ShiftGroupLeft_OtherGroup (CRASHED) This is because the DOM Distiller feature is not enabled as expected (for Brave) on startup, and when AdsTabHelper::RunIsolatedJavaScript() (called via the WebContentsObserver machinery) calls dom_distiller::RunIsolatedJavaScript() it will fail the DCHECK that makes sure the JS World ID is already set. To fix this, we move the code of BraveMainDelegate::BasicStartupComplete() to ChromeMainDelegate::BasicStartupComplete() via a chromium_src override, so that we make sure that exactly the same initialization done for Brave and Brave-related browser tests also applies when running upstrem browser tests. This does not only fix that crash, but also makes sure that upstream browser tests are run with the same features as Brave, which is the right thing to do.
LorenzoMinto
pushed a commit
that referenced
this pull request
Nov 10, 2022
# This is the 1st commit message: Add federated client to BraveFederated component # This is the commit message #2: Remove unnecessary files # This is the commit message #3: Hook up learning service # This is the commit message #4: Unlink client with DataStore # This is the commit message #5: comment # This is the commit message #6: temp # This is the commit message #7: todos # This is the commit message #8: Change to follow Chromium styleguide # This is the commit message #9: Refactor 1 # This is the commit message #10: Use grpc_support for comms
LorenzoMinto
pushed a commit
that referenced
this pull request
Dec 12, 2022
# This is the 1st commit message: Add federated client to BraveFederated component # This is the commit message #2: Remove unnecessary files # This is the commit message #3: Hook up learning service # This is the commit message #4: Unlink client with DataStore # This is the commit message #5: comment # This is the commit message #6: temp # This is the commit message #7: todos # This is the commit message #8: Change to follow Chromium styleguide # This is the commit message #9: Refactor 1 # This is the commit message #10: Use grpc_support for comms
LorenzoMinto
pushed a commit
that referenced
this pull request
Jan 25, 2023
# This is the 1st commit message: Add federated client to BraveFederated component # This is the commit message #2: Remove unnecessary files # This is the commit message #3: Hook up learning service # This is the commit message #4: Unlink client with DataStore # This is the commit message #5: comment # This is the commit message #6: temp # This is the commit message #7: todos # This is the commit message #8: Change to follow Chromium styleguide # This is the commit message #9: Refactor 1 # This is the commit message #10: Use grpc_support for comms
LorenzoMinto
pushed a commit
that referenced
this pull request
Feb 14, 2023
# This is the 1st commit message: Add federated client to BraveFederated component # This is the commit message #2: Remove unnecessary files # This is the commit message #3: Hook up learning service # This is the commit message #4: Unlink client with DataStore # This is the commit message #5: comment # This is the commit message #6: temp # This is the commit message #7: todos # This is the commit message #8: Change to follow Chromium styleguide # This is the commit message #9: Refactor 1 # This is the commit message #10: Use grpc_support for comms
fallaciousreasoning
added a commit
that referenced
this pull request
Jun 15, 2023
# This is the 1st commit message: Fix cr114 changes # This is the commit message #2: Begin work on GeoLocationClientObject # This is the commit message #3: Add ScopedBlockingCall # This is the commit message #4: Use AccuracyLevel # This is the commit message #5: WIP locationupdated signal # This is the commit message #6: Formatting # This is the commit message #7: Move initialization into the GeoClueClientObject # This is the commit message #8: Revert to CHECK
fallaciousreasoning
added a commit
that referenced
this pull request
Jul 24, 2023
# This is the 1st commit message: Fix cr114 changes # This is the commit message #2: Begin work on GeoLocationClientObject # This is the commit message #3: Add ScopedBlockingCall # This is the commit message #4: Use AccuracyLevel # This is the commit message #5: WIP locationupdated signal # This is the commit message #6: Formatting # This is the commit message #7: Move initialization into the GeoClueClientObject # This is the commit message #8: Revert to CHECK
AlexeyBarabash
added a commit
that referenced
this pull request
May 31, 2024
AlexeyBarabash
added a commit
that referenced
this pull request
Jun 7, 2024
AlexeyBarabash
added a commit
that referenced
this pull request
Jun 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.