New issue
Advanced search Search tips

Issue 810179 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

NPE from accessing WebContentsUserData

Project Member Reported by jinsuk...@chromium.org, Feb 7 2018

Issue description

the clusterfuzz report (https://clusterfuzz.com/testcase?key=4608926788878336) contains a Java exception:

java.lang.NullPointerException: Attempt to invoke virtual method 'java.util.Map org.chromium.content.browser.webcontents.WebContentsImpl.getUserDataMap()' on a null object reference
at org.chromium.content.browser.webcontents.WebContentsUserData.fromWebContents(WebContentsUserData.java:5)
at org.chromium.content.browser.selection.SelectionPopupControllerImpl.fromWebContents(SelectionPopupControllerImpl.java:19)
at org.chromium.content.browser.ContentViewCoreImpl.destroyPastePopup(ContentViewCoreImpl.java:577)
at org.chromium.content.browser.ContentViewCoreImpl$ContentGestureStateListener.onSingleTap$51D2ILG_0(ContentViewCoreImpl.java:17)
at org.chromium.content.browser.GestureListenerManagerImpl.onSingleTapEventAck(GestureListenerManagerImpl.java:38)

There have been many other instances like this. In addition to resolving the particular instance reported through clusterfuzz, overall access to WebContentsUserData-backed object should be examined, and if possible, figure out a good way (maybe better than excessive null checking) to avoid future exceptions.


 
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/757e8982a8780763af9ed7cd61c1853e89b5f58c

commit 757e8982a8780763af9ed7cd61c1853e89b5f58c
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Mon Feb 12 01:16:19 2018

Tidy up null checking against WebContents

WebContentsUserData-backed objects requires its WebContents instance from
CVC be non-null, which causes numerous null checking statments. This CL
examines callsites to the methods that contains accesses to those
objects, adds null-checking statements if missing, and removes it
if redundant.

Some observations the changes are based on:

- private native method doesn't need null check; WebContents is
    certainly valid at that point.
- CVC.destroy() itself or calls from it (before it nulls out WebContents)
    doesn't need null check since WebContent is valid up to that point.

Added null check:
   ... since clusterfuzz results warn that these need it:
    - setTouchScrollInProgress
    - destroyPastePopup

   ... by moving it over from the 2 callsites:
    - updateTextSelectionUI (onAttachedToWindow, onDetachedFromWindow)

Removed null check as callsites already have null checking/assert,
      or assure WebContents is not null:
 - hidePopupsAndClearSelection (WebContentObserver.resetPopupsAndInput,
                                onFocusChanged, showSelectPopup)
 - hidePopupsAndPreserveSelection (destroy, onHide, updateTextSelectionUI,
                                   onFocusChanged, onRotationChanged)
 - hidePopups (above 2)
 - onAttachedToWindow, onDetachedFromWindow: moved to updateTextSelectionUI
 - onRotationChanged: consolidated null checks

Inlined one-liner restoreSelectionPopupsIfNecessary into callsites
 to eliminate the concern on null check on it (much clearer when inlined).

Bug:  810179 
Change-Id: I55aab2b1f7917a9af4522a1bcf8d3e7d9b172f2a
Reviewed-on: https://chromium-review.googlesource.com/907770
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536016}
[modify] https://crrev.com/757e8982a8780763af9ed7cd61c1853e89b5f58c/content/public/android/java/src/org/chromium/content/browser/ContentViewCoreImpl.java

Project Member

Comment 2 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc5ff80f49b12b9eddc0334914081bcb496fd00e

commit fc5ff80f49b12b9eddc0334914081bcb496fd00e
Author: Jinsuk Kim <jinsukkim@chromium.org>
Date: Wed Feb 14 06:43:46 2018

Fix NPE in processing gesture ack events

GestureListenerManager should stop receiving gesture ack events
from native if ContentViewCore is in the destroyed state. This CL
disconnects the link between native and Java side of
GestureListenerManagerImpl when that happens.

Bug: 803244,  810179 
Change-Id: If414dbd44e008fc3b4196d04c13838bc682d49b7
Reviewed-on: https://chromium-review.googlesource.com/915321
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Jinsuk Kim <jinsukkim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536666}
[modify] https://crrev.com/fc5ff80f49b12b9eddc0334914081bcb496fd00e/content/browser/android/gesture_listener_manager.cc
[modify] https://crrev.com/fc5ff80f49b12b9eddc0334914081bcb496fd00e/content/browser/android/gesture_listener_manager.h
[modify] https://crrev.com/fc5ff80f49b12b9eddc0334914081bcb496fd00e/content/public/android/java/src/org/chromium/content/browser/ContentViewCoreImpl.java
[modify] https://crrev.com/fc5ff80f49b12b9eddc0334914081bcb496fd00e/content/public/android/java/src/org/chromium/content/browser/GestureListenerManagerImpl.java

Status: Fixed (was: Assigned)

Sign in to add a comment