Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type error in webview's getMessageIdFromNode #2751

Closed
borisyankov opened this issue Jul 2, 2018 · 4 comments
Closed

Type error in webview's getMessageIdFromNode #2751

borisyankov opened this issue Jul 2, 2018 · 4 comments

Comments

@borisyankov
Copy link
Contributor

Webview throws an exception: msgNode.getAttribute is not a function.

This happens both on Android and iOS (the only other known issue is Android-specific)

@borisyankov borisyankov self-assigned this Jul 2, 2018
@borisyankov borisyankov added the bug label Jul 2, 2018
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Aug 1, 2018
Fixes zulip#2538 zulip#2505 zulip#2538 zulip#2558 zulip#2751

When using `postMessage` to communicate with the WebView, depending
on the content of the message we send, an exception might be thrown.

A minimal but reliable way to reproduce the issue is to send
the string `'%22'`. Other character combinations might also cause issues.

Messages are sent to the WebView in different ways for iOS/Android.
This explains why the issue this is fixing is Android-specific.

In iOS, a custom navigation scheme is used to pass the data into
the webview (the RCTJSNavigationScheme constant one can grep for)

More interesting source code reading on that can be done if one
looks at `webViewDidFinishLoad` in `RTCWebView.m`.

The Android messaging happens in `receiveCommand` in `ReactWebViewManager.java`
It is passed by navigating to a URL of the type `javascript:(.....)`
which is a standard way of injecting JavaScript into webpages.

The issue comes from the fact that `loadUrl` since Android 4.4 does
an URL decode on the string passed to it:

https://developer.android.com/reference/android/webkit/WebView#loadUrl(java.lang.String)

`evaluateJavascript` does not:

https://developer.android.com/reference/android/webkit/WebView.html#evaluateJavascript(java.lang.String,%20android.webkit.ValueCallback%3Cjava.lang.String%3E)
@gnprice
Copy link
Member

gnprice commented Aug 3, 2018

Reported again yesterday, with a bit more detail. Screenshot:

image

This was on 15.0.92 (current latest), on iOS.

From a quick look at the code, this is in getMessageIdFromNode:

const getMessageIdFromNode = (node: Node): number => {
  const msgNode = getMessageNode(node);
  return msgNode ? +msgNode.getAttribute('data-msg-id') : -1;
};

So somehow we're getting a msgNode that is truthy, but doesn't have a getAttribute method.

@gnprice gnprice changed the title Webview sometimes throws an exception Type error in webview's getMessageIdFromNode Aug 3, 2018
@gnprice
Copy link
Member

gnprice commented Aug 3, 2018

Oh also: asked what they were doing just before the error, the person who reported it says:

I just went to a stream and then a topic and I happened to tap the screen accidentally though I don’t think that would have anything to do with the error

Several call sites of getMessageIdFromNode are in our click event listener, so those are a likely suspect.

@chrisbobbe
Copy link
Contributor

It looks like 65e3ea3 types node as ?Node instead of Node, but this wouldn't catch the problem Greg pointed out:

So somehow we're getting a msgNode that is truthy, but doesn't have a getAttribute method.

Also, getMessageIdFromNode was deprecated in 7f968e5, but it still has six call sites.

@gnprice
Copy link
Member

gnprice commented Feb 13, 2020

Looking at Sentry events, we've had no cases of this in (at least) the past 90 days:
https://sentry.io/organizations/zulip/events/?project=191284&query=%22msgNode.getAttribute%22&statsPeriod=90d

To try to confirm that's not just a lack of data: the way these would get reported to Sentry is that the same window.onerror which puts up that red banner (or a less detailed one, in release builds) also posts an event through sendMessage, which gets processed here in webViewEventHandlers.js:

    case 'error':
      logging.error(`WebView exception: ${JSON.stringify(event.details)}`);
      break;

And we do get other exceptions reported through that path:
https://sentry.io/organizations/zulip/events/?project=191284&query=%22WebView+exception%22+%22TypeError%22&statsPeriod=14d

So it seems this got fixed at some point. 🤷‍♂️ If we hear a fresh report, we'll reopen.

@gnprice gnprice closed this as completed Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants