Skip to content

Commit

Permalink
fix: Fix unreleased stack overflow on statechanged (#5712)
Browse files Browse the repository at this point in the history
Do not dispatch statechanged events unless there is an actual change.
This fixes a stack overflow in the Cast proxy code triggered by
statechanged events.
  • Loading branch information
joeyparrish authored Oct 3, 2023
1 parent 1678cee commit ebacf32
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 6 deletions.
10 changes: 6 additions & 4 deletions lib/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -5737,11 +5737,13 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
} else if (this.video_.ended) {
updateState = 'ended';
}
history.update(updateState);
const stateChanged = history.update(updateState);

const eventName = shaka.util.FakeEvent.EventName.StateChanged;
const data = (new Map()).set('newstate', updateState);
this.dispatchEvent(this.makeEvent_(eventName, data));
if (stateChanged) {
const eventName = shaka.util.FakeEvent.EventName.StateChanged;
const data = (new Map()).set('newstate', updateState);
this.dispatchEvent(this.makeEvent_(eventName, data));
}
}

/**
Expand Down
8 changes: 6 additions & 2 deletions lib/util/state_history.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,13 +40,15 @@ shaka.util.StateHistory = class {

/**
* @param {string} state
* @return {boolean} True if this changed the state
*/
update(state) {
// |open_| will only be |null| when we first call |update|.
if (this.open_ == null) {
this.start_(state);
return true;
} else {
this.update_(state);
return this.update_(state);
}
}

Expand Down Expand Up @@ -116,6 +118,7 @@ shaka.util.StateHistory = class {

/**
* @param {string} state
* @return {boolean} True if this changed the state
* @private
*/
update_(state) {
Expand All @@ -131,7 +134,7 @@ shaka.util.StateHistory = class {

// If the state has not changed, there is no need to add a new entry.
if (this.open_.state == state) {
return;
return false;
}

// We have changed states, so "close" the open state.
Expand All @@ -142,6 +145,7 @@ shaka.util.StateHistory = class {
state: state,
duration: 0,
};
return true;
}

/**
Expand Down
47 changes: 47 additions & 0 deletions test/cast/cast_receiver_integration.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,53 @@ filterDescribe('CastReceiver', castReceiverIntegrationSupport, () => {
}
});

describe('state changed event', () => {
it('does not trigger a stack overflow', async () => {
const p = waitForLoadedData();

// We had a regression in which polling attributes eventually triggered a
// stack overflow because of a state change event that fired every time
// we checked the state. The error itself got swallowed and hidden
// inside CastReceiver, but would cause tests to disconnect in some
// environments (consistently in GitHub Actions VMs, inconsistently
// elsewhere).
//
// Testing for this is subtle: if we try to catch the error, it will be
// caught at a point when the stack has already or is about to overflow.
// Then if we call "fail", Jasmine will grow the stack further,
// triggering *another* overflow inside of fail(), causing our test to
// *pass*. So we need to fail fast. The best way I have found is to
// catch the very first recursion of pollAttributes_, long before we
// overflow, fail, then return early to avoid the actual recursion.
const original = /** @type {?} */(receiver).pollAttributes_;
let numRecursions = 0;
// eslint-disable-next-line no-restricted-syntax
/** @type {?} */(receiver).pollAttributes_ = function() {
try {
if (numRecursions > 0) {
fail('Found recursion in pollAttributes_!');
return undefined;
}

numRecursions++;
return original.apply(receiver, arguments);
} finally {
numRecursions--;
}
};

// Start the process of loading by sending a fake init message.
fakeConnectedSenders(1);
fakeIncomingMessage({
type: 'init',
initState: fakeInitState,
appData: {},
}, mockShakaMessageBus);

await p;
});
});

describe('without drm', () => {
it('sends reasonably-sized updates', async () => {
// Use an unencrypted asset.
Expand Down

0 comments on commit ebacf32

Please sign in to comment.