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-10093] Android: HTTPClient leaks function callbacks (ex: onload, ondatastream) #2687

Merged
merged 5 commits into from
Aug 8, 2012

Conversation

joshthecoder
Copy link
Contributor

This resolves a memory leak caused by the HTTPClient retaining a reference
to the JavaScript callback functions. The result would be a circular reference
of the HTTPClient (its retained by the callback function's scope).

I introduce a new API that allows calling properties on a proxy
object that are function from Java. This works around the circular reference issue
since it requires no extra referencing since the callbacks are kept alive anyway
since they are properties of the proxy.

See the JIRA ticket for a test case and instructions.

Joshua Roesslein added 4 commits August 7, 2012 00:33
Exposes a direct method for calling a function referenced
by an object property. Requires no additonal reference management
since the object will retain the function for us. In most callback
use cases this is okay and avoids circular reference bugs.
This wraps around KrollObject.callProperty() to make
it async. and also thread safe.
@joshthecoder
Copy link
Contributor Author

A friendly reminder be sure to test on both V8 and Rhino. Also verify emulator and
device have no special edge cases due to the JNI reference differences between them.

@billdawson
Copy link
Contributor

FR accepted. Huge improvement. Tested rhino+v8 on Galaxy Nexus (4.1) and HTC Desire (2.2), then also rhino+v8 on the 2.3 emulator.

Makes me wish we didn't have event listener collections stored on the java side and could also just use methods-as-properties. But of course in the case of event listeners we need to support n listeners per event. :(

CR almost okay, except our java standard wasn't followed with respect to function opening brace being on next line. This happened to me also recently because I was using Eclipse Juno and had forgotten to port the auto format settings over from Galileo.

After those braces are fixed I'll accept.

@joshthecoder
Copy link
Contributor Author

@billdawson I think all new code follows the Java coding standard plus a few tweaks documented in our wiki.
I don't see anything about amending the standard to place the opening brace on the next line.

While I don't have a preference either way, we should update the wiki if this is the practice we want to keep.

@joshthecoder
Copy link
Contributor Author

On the topic of event listeners we do dispatch to the JS listeners with one native call (nativeFireEvent).
The only reason we have collections on the Java side is for any Java based listeners (ex: webview).

@billdawson
Copy link
Contributor

Ah, okay. I just assumed it was in the standard because it has been our
"standard" (de facto, apparently) for quite a while now. I'm surprised you
ever sneaked it past Marshall. :)

Yes, please change them, and I'll update the standard.

On Wed, Aug 8, 2012 at 8:30 PM, Joshua Roesslein
notifications@github.comwrote:

@billdawson https://github.com/billdawson I think all new code follows
the Java coding standard plus a few tweaks documented in our wiki.
I don't see anything about amending the standard to place the opening
brace on the next line.

While I don't have a preference either way, we should update the wiki if
this is the practice we want to keep.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2687#issuecomment-7592710.

@billdawson
Copy link
Contributor

per discussion, no change needed (it's not part of our standard, my bad.)

CR also accepted. Merging...

billdawson added a commit that referenced this pull request Aug 8, 2012
[TIMOB-10093] Android: HTTPClient leaks function callbacks (ex: onload, ondatastream)
@billdawson billdawson merged commit 77023f0 into tidev:master Aug 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants