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

[TIMOB-25242] Android: Improve validation in filenameExists() #9422

Merged
merged 3 commits into from Nov 15, 2017

Conversation

garymathews
Copy link
Contributor

  • Prevent filenameExists() from crashing when fileIndex is null

JIRA Ticket

@garymathews garymathews added this to the 7.0.0 milestone Sep 12, 2017
@jpriebe
Copy link

jpriebe commented Sep 21, 2017

Questions: does the fact that JSON.parse("index.json") does not return an object indicate a larger problem?

If we're trying to load a module that should exist and was built into the app, but this filenameExists() call returns null, that could be almost as bad as the V8Exception.

And wouldn't any subsequent calls to filenameExists() for any files just return null because you're missing this critical index file?

@jpriebe
Copy link

jpriebe commented Sep 21, 2017

We hit the exception in question intermittently in our app. We'll hit it, then launch the app again, and we don't hit it.

Assuming that index.json is built at compile time, then it clearly exists on the device even when the exception is being thrown. So it's probably not so much an issue of the index.json not existing, but more of a race condition of sorts.

I don't know if that means there should be some kind of retry mechanism when you can't parse index.json.

At any rate, a casual inspection of the code would suggest that if index.json is not parseable, there's no way to require() a JS module. So that's pretty much going to kill the app.

@@ -617,6 +617,9 @@ Module.prototype.filenameExists = function (filename) {
if (!fileIndex) {
var json = assets.readAsset('index.json');
fileIndex = JSON.parse(json);
if (!fileIndex) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to explicitly return a Boolean in this method. I understand that null is "truthy" and evaluates to false, but I'd strongly prefer we simply alter the return statement below to be:

return filename && (filename in fileIndex);

@sgtcoolguy
Copy link
Contributor

@jpriebe Yes, being unable to load index.json is indicative of a larger problem. That file is generated at build time with the listing of assets in the app. If that file is missing or corrupt, I don't believe you'd be able to require any file.

@sgtcoolguy
Copy link
Contributor

So taking a quick look, my best guess here is that the assets.readAsset('index.json'); call must be returning null, since that's how JSON.parse(json); would return null. If you had empty string I believe it throws a SyntaxError. So I think the underlying bug is likely to be in the asset manager stuff, and given that it's intermittent, @jpriebe is likely right it's some sort of app/runtime startup race condition.

@garymathews
Copy link
Contributor Author

@sgtcoolguy Thanks for taking a look, updated PR!

@gimdongwoo
Copy link
Contributor

Any update?

@gimdongwoo
Copy link
Contributor

gimdongwoo commented Nov 3, 2017

This commit has seriously bugs.

[ERROR] TiExceptionHandler: (main) [194,194] ----- Titanium Javascript Runtime Error -----
[ERROR] TiExceptionHandler: (main) [0,194] - In ti:/module.js:305,2
[ERROR] TiExceptionHandler: (main) [0,194] - Message: Uncaught Error: Requested module not found: av.imageview
[ERROR] TiExceptionHandler: (main) [0,194] - Source: 	throw new Error("Requested module not found: " + request);
...
[INFO]  art: Rejecting re-init on previously-failed class java.lang.Class<com.onesignal.OneSignalChromeTab$OneSignalCustomTabsServiceConnection>: java.lang.NoClassDefFoundError: Failed resolution of: Landroid/support/customtabs/CustomTabsServiceConnection;

Errors are related native module

@lokeshchdhry
Copy link
Contributor

FR Passed.

No V8 exceptions seen & module loading works fine, checked with using ti.paint module in the app.

Studio Ver: 4.10.0.201709271713
SDK Ver: 7.0.0 local build
OS Ver: 10.12.3
Xcode Ver: Xcode 8.3.3
Appc NPM: 4.2.10
Appc CLI: 6.3.0
Ti CLI Ver: 5.0.14
Alloy Ver: 1.10.7
Node Ver: 7.10.1
Java Ver: 1.8.0_101
Devices: ⇨ google Nexus 6P --- Android 8.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants