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

Issue 603303002: Fix crash when showing multiple popups. (Closed)

Created:
6 years, 2 months ago by Ted C
Modified:
6 years, 2 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Fix crash when showing multiple popups. The problem is that calling hide/cancel on a popup is async. With two popups, you would get Previously: SHOW A HIDE A SHOW B NOTIFY SELECTION A HIDE B NOTIFY SELECTION B When notifying the selection of A in the above scenario, it was clearing the frame pointer (and actually updating the wrong popup). Then when hiding the second popup, it's reference was actually gone already. Fix this by making the call to hide sync instead of async. BUG=416354 Committed: https://crrev.com/52516a4c58a6ec6a88452601e2efea6e2f75704a Cr-Commit-Position: refs/heads/master@{#296826}

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -16 lines) Patch
M content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java View 1 chunk +3 lines, -1 line 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDialog.java View 7 chunks +15 lines, -7 lines 1 comment Download
M content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java View 4 chunks +11 lines, -8 lines 0 comments Download

Messages

Total messages: 9 (2 generated)
Ted C
PTAL
6 years, 2 months ago (2014-09-25 21:38:01 UTC) #2
jdduke (slow)
lgtm. I don't suppose we have any existing tests for this code that could be ...
6 years, 2 months ago (2014-09-25 21:43:30 UTC) #3
Avi (use Gerrit)
This looks plausible but I don't know the internals well enough to say. I'm happy ...
6 years, 2 months ago (2014-09-25 21:43:30 UTC) #4
Avi (use Gerrit)
Oh, crossed paths. LGTM!
6 years, 2 months ago (2014-09-25 21:44:01 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603303002/1
6 years, 2 months ago (2014-09-25 22:31:40 UTC) #7
commit-bot: I haz the power
Committed patchset #1 (id:1) as 31f4041fefdf529bf9b9aa78fe7a86d4e4bb1577
6 years, 2 months ago (2014-09-25 23:24:57 UTC) #8
commit-bot: I haz the power
6 years, 2 months ago (2014-09-25 23:27:22 UTC) #9
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/52516a4c58a6ec6a88452601e2efea6e2f75704a
Cr-Commit-Position: refs/heads/master@{#296826}

Powered by Google App Engine
This is Rietveld 408576698