Race condition in JS #29

Closed
hrj opened this Issue Dec 25, 2014 · 4 comments

Comments

Projects
None yet
1 participant
@hrj
Member

hrj commented Dec 25, 2014

We had to add the debugger to the window context factory in order to workaround a race condition in the JS library.

But it is commented out in the ugly patches because it seemed to be not required anymore.

However, the race condition appeared again today.

Quick testing shows that using a dummy debugger works fine. But needs more testing.

TODO

  1. Try to make a reproducible test-case for the race condition
  2. Check if a dummy debugger can be used. If so, remove the extra permission from the ugly patch set.

@hrj hrj modified the milestones: 0.2, 0.3 Dec 25, 2014

@hrj

This comment has been minimized.

Show comment
Hide comment
@hrj

hrj Jan 3, 2015

Member

Tests seem to be working fine without the debugger. Removed the extra permission from ugly patches branch.

Member

hrj commented Jan 3, 2015

Tests seem to be working fine without the debugger. Removed the extra permission from ugly patches branch.

@hrj hrj closed this Jan 3, 2015

@hrj

This comment has been minimized.

Show comment
Hide comment
@hrj

hrj Jan 5, 2015

Member

The problem surfaced again on Reddit 😞

The good news is that the problem seemed to go away after using a dummy debugger that returns dummy frames (instead of just null frames). If this is confirmed, no extra permissions would be required.

Have opened an upstream thread

Member

hrj commented Jan 5, 2015

The problem surfaced again on Reddit 😞

The good news is that the problem seemed to go away after using a dummy debugger that returns dummy frames (instead of just null frames). If this is confirmed, no extra permissions would be required.

Have opened an upstream thread

@hrj hrj reopened this Jan 5, 2015

@hrj

This comment has been minimized.

Show comment
Hide comment
@hrj

hrj Jan 7, 2015

Member

I was able to pin down the root-cause. There is code to set the parent scope of an object, when that object is being returned to JS. Sometimes the window object is returned and its parent scope is set to a non-null scope. However, this causes a circular chain of parent-scopes as some of those scopes might themselves have a parent-scope set to window.

I was able to determine atleast one instance where this happens, and coded it to a test-case

I am yet to code the part that causes the infinite loop (if there is no fix to the first part). Anything that walks the parent scope chain should work.

A fix has been committed to the ugly-patches branch. It consists of a simple check before setting the parent-scope of window.

Member

hrj commented Jan 7, 2015

I was able to pin down the root-cause. There is code to set the parent scope of an object, when that object is being returned to JS. Sometimes the window object is returned and its parent scope is set to a non-null scope. However, this causes a circular chain of parent-scopes as some of those scopes might themselves have a parent-scope set to window.

I was able to determine atleast one instance where this happens, and coded it to a test-case

I am yet to code the part that causes the infinite loop (if there is no fix to the first part). Anything that walks the parent scope chain should work.

A fix has been committed to the ugly-patches branch. It consists of a simple check before setting the parent-scope of window.

@hrj hrj closed this Jan 7, 2015

@hrj hrj changed the title from Check if special permissions for js lib are required to Race condition in JS Jul 17, 2015

@hrj hrj removed the need-triage label Jul 17, 2015

@hrj

This comment has been minimized.

Show comment
Hide comment
@hrj

hrj Jul 17, 2015

Member

This is now being merged in master. For future reference: the fix is in JavaObjectWrapper.setParentScope.

Member

hrj commented Jul 17, 2015

This is now being merged in master. For future reference: the fix is in JavaObjectWrapper.setParentScope.

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