[Side Panel] Fix dangling WebContents pointer in ReadingListPageHandler [chromium/src : main]
Eshwar Stalin (Gerrit)
Eshwar Stalin added 1 comment![]( "Open in Gerrit")
base::WeakPtr<content::WebContents> web_contents_;Member variables should never be WeakPtrs. That's really against recommended usage of WeakPtr. We should fix lifecycle issues instead of turning things into Weakptrs.
Related details
- Caroline Rising
- Darryl James
- Foromo Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
base::WeakPtr<content::WebContents> web_contents_;Member variables should never be WeakPtrs. That's really against recommended usage of WeakPtr. We should fix lifecycle issues instead of turning things into Weakptrs.
Ok great, Got it.
The main issue currently is that `ReadingListModel` outlives the `webui->WebContents()`, and synchronous model updates can fire while the browser/tab is shutting down.
I think alternatively to using a WeakPtr, stop observing the `ReadingListModel` when the WebContents is destroyed should address this.
WDYT?
Related details
- Caroline Rising
- Darryl James
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 1 comment![]( "Open in Gerrit")
base::WeakPtr<content::WebContents> web_contents_;Foromo Daniel SoromouMember variables should never be WeakPtrs. That's really against recommended usage of WeakPtr. We should fix lifecycle issues instead of turning things into Weakptrs.
Ok great, Got it.
The main issue currently is that `ReadingListModel` outlives the `webui->WebContents()`, and synchronous model updates can fire while the browser/tab is shutting down.
I think alternatively to using a WeakPtr, stop observing the `ReadingListModel` when the WebContents is destroyed should address this.
WDYT?
- Caroline Rising
- Darryl James
- Foromo Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
base::WeakPtr<content::WebContents> web_contents_;Foromo Daniel SoromouMember variables should never be WeakPtrs. That's really against recommended usage of WeakPtr. We should fix lifecycle issues instead of turning things into Weakptrs.
Eshwar StalinOk great, Got it.
The main issue currently is that `ReadingListModel` outlives the `webui->WebContents()`, and synchronous model updates can fire while the browser/tab is shutting down.
I think alternatively to using a WeakPtr, stop observing the `ReadingListModel` when the WebContents is destroyed should address this.
WDYT?
Yeah that's the correct way to fix this.
- Caroline Rising
- Darryl James
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin added 2 comments![]( "Open in Gerrit")
CHECK(web_contents());I don't see anything that guarentees the web contents will always be valid when this is called. In other places where we CHECK(web_contents()) we ensure we unregister observing reading_list_model, but UpdateCurrentPageActionButton gets called in other places, so I think it might be safer to add nullptr checks for this?
Related details
- Caroline Rising
- Darryl James
- Foromo Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou added 1 comment![]( "Open in Gerrit")
CHECK(web_contents());I don't see anything that guarentees the web contents will always be valid when this is called. In other places where we CHECK(web_contents()) we ensure we unregister observing reading_list_model, but UpdateCurrentPageActionButton gets called in other places, so I think it might be safer to add nullptr checks for this?
The other place where it's called is in `SetActiveURL` which get invoke from `ReadingListUI::SetActiveTabURL`. `ReadingListUI` outlive it `web_contents` and `handler`, so I would be expecting that `web_contents` should be valid.
Related details
- Caroline Rising
- Darryl James
- Eshwar Stalin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Eshwar Stalin
| Code-Review | +1 |
1 comment
CHECK(web_contents());Foromo Daniel SoromouI don't see anything that guarentees the web contents will always be valid when this is called. In other places where we CHECK(web_contents()) we ensure we unregister observing reading_list_model, but UpdateCurrentPageActionButton gets called in other places, so I think it might be safer to add nullptr checks for this?
The other place where it's called is in `SetActiveURL` which get invoke from `ReadingListUI::SetActiveTabURL`. `ReadingListUI` outlive it `web_contents` and `handler`, so I would be expecting that `web_contents` should be valid.
- Caroline Rising
- Darryl James
- Foromo Daniel Soromou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Foromo Daniel Soromou (Gerrit)
Foromo Daniel Soromou voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Caroline Rising
- Darryl James
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Side Panel] Fix dangling WebContents pointer in ReadingListPageHandler
This patch resolves a dangling pointer issue in ReadingListPageHandler
by replacing the `DanglingUntriaged` raw pointer to
`content::WebContents` with a `base::WeakPtr<content::WebContents>`.
Because `WebContents` can be destroyed before the
`ReadingListPageHandler` (e.g., during teardown), accessing the dangling
raw pointer could lead to Use-After-Free (UAF) bugs . Null checks have
been added to safely return early in cases where `web_contents_` has
already been invalidated.
- M chrome/browser/ui/webui/side_panel/reading_list/reading_list_page_handler.cc
- M chrome/browser/ui/webui/side_panel/reading_list/reading_list_page_handler.h
Code-Review: +1 by Eshwar Stalin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |