Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(343)

Issue 2616633003: Fix crash in StaticHTMLNativeContent when deallocating WKWebView on iOS9 (Closed)

Created:
3 years, 11 months ago by Olivier
Modified:
3 years, 11 months ago
CC:
chromium-reviews, pkl (ping after 24h if needed), jam, noyau+watch_chromium.org, darin-cc_chromium.org, stkhapugin, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix crash in StaticHTMLNativeContent when deallocating WKWebView on iOS9 Reset the scrollview delegate before releasing the WKWebView. Move the |scrollview setDelegate:| to static_html_native_content.mm for symetry. BUG=678118 Review-Url: https://codereview.chromium.org/2616633003 Cr-Commit-Position: refs/heads/master@{#441967} Committed: https://chromium.googlesource.com/chromium/src/+/e1e1e253bf95fad3c1311da89a2927327fb6b17a

Patch Set 1 #

Patch Set 2 : move setDelegate #

Total comments: 1

Patch Set 3 : no dismiss -> close call #

Total comments: 4

Patch Set 4 : setDelegate in dealloc #

Unified diffs Side-by-side diffs Delta from patch set Stats (+7 lines, -1 line) Patch
M ios/chrome/browser/ui/browser_view_controller.mm View 1 1 chunk +0 lines, -1 line 0 comments Download
M ios/chrome/browser/ui/reading_list/offline_page_native_content.mm View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M ios/chrome/browser/ui/static_content/static_html_native_content.mm View 1 2 3 2 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (5 generated)
Olivier
3 years, 11 months ago (2017-01-04 13:52:38 UTC) #3
Eugene But (OOO till 7-30)
https://codereview.chromium.org/2616633003/diff/20001/ios/chrome/browser/ui/static_content/static_html_native_content.mm File ios/chrome/browser/ui/static_content/static_html_native_content.mm (right): https://codereview.chromium.org/2616633003/diff/20001/ios/chrome/browser/ui/static_content/static_html_native_content.mm#newcode100 ios/chrome/browser/ui/static_content/static_html_native_content.mm:100: [self willBeDismissed]; According to |willBeDismissed| comments this is called ...
3 years, 11 months ago (2017-01-04 17:52:18 UTC) #4
Olivier
Is this solution better?
3 years, 11 months ago (2017-01-04 18:26:33 UTC) #5
Eugene But (OOO till 7-30)
https://codereview.chromium.org/2616633003/diff/40001/ios/chrome/browser/ui/reading_list/offline_page_native_content.mm File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2616633003/diff/40001/ios/chrome/browser/ui/reading_list/offline_page_native_content.mm#newcode96 ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: if (!item || item->GetVirtualURL() != [self virtualURL]) { Could ...
3 years, 11 months ago (2017-01-04 21:24:38 UTC) #6
Olivier
Thanks. I moved to dealloc as suggested. https://codereview.chromium.org/2616633003/diff/40001/ios/chrome/browser/ui/reading_list/offline_page_native_content.mm File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right): https://codereview.chromium.org/2616633003/diff/40001/ios/chrome/browser/ui/reading_list/offline_page_native_content.mm#newcode96 ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:96: if (!item ...
3 years, 11 months ago (2017-01-06 10:03:24 UTC) #7
Eugene But (OOO till 7-30)
lgtm
3 years, 11 months ago (2017-01-06 17:32:21 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2616633003/60001
3 years, 11 months ago (2017-01-06 17:34:44 UTC) #10
commit-bot: I haz the power
3 years, 11 months ago (2017-01-06 17:45:53 UTC) #13
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e1e1e253bf95fad3c1311da89a29...

Powered by Google App Engine
This is Rietveld 408576698