638617 - Warnings at nsOggReader:946 while seeking

Status: RESOLVED FIXED

Description

[Chris Pearce [:cpearce (GMT+13)]]

I sometimes the NS_ENSURE_SUCCESS at http://mxr.mozilla.org/mozilla-central/source/content/media/ogg/nsOggReader.cpp#946 is failing and producing a warning while seeking. e.g.:

0[6de7a8]: WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file c:/Users/cpearce
/src/blue/objdir/content/media/ogg/../../../../content/media/ogg/nsOggReader.cpp, line 956
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file c:/Users/cpearce/src/blue/o
bjdir/content/media/ogg/../../../../content/media/ogg/nsOggReader.cpp, line 956
0[6de7a8]: WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file c:/Users/cpearce
/src/blue/objdir/content/media/ogg/../../../../content/media/ogg/nsOggReader.cpp, line 956
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file c:/Users/cpearce/src/blue/o
bjdir/content/media/ogg/../../../../content/media/ogg/nsOggReader.cpp, line 956

(Note my output has line 956 because I have patches from bug 580531 applied.)

This is caused by our call to nsMediaStream::ReadFromCache() (the result of which we're checking here) trying to read before the start of cached data. This is because nsOggReader::FindEndTime(PRInt64,PRBool,ogg_sync_state*) steps backwards in 5KB chunks from the end of cached data trying to read an ogg page with a timestamp. If it backsteps far enough it ends up trying to read before the start of cached data, that read fails, and we get this warning.

We should cap the read in FindEndTime() to not try to read before the start of cached data, and exit gracefully when FindEndTime() back-steps to the start of cached data without finding an end time.

We also shouldn't be returning an nsresult error code from FindEndTime() on failure; it returns a PRInt64, and callers expect it to return -1 on failure.

Patch to follow.

Comment 1

[Chris Pearce [:cpearce (GMT+13)]]

Attachment: Patch: Ensure nsOggReader::FindEndTime() doesn't read outside of cached data bounds, and reports errors properly

* Add aStartOffset parameter to nsOggReader::FindEndTime() so that we can be sure not to read before the start of cached data.
* Return -1 instead of an nsresult upon failure in FindEndTime().
* Based on top of the patches from Bug 580531.

Comment 2

[Chris Pearce [:cpearce (GMT+13)]]

http://hg.mozilla.org/mozilla-central/rev/47ca9ea23fc3

Comment 3

[Anthony Hughes (:ashughes) [QA]]

Can someone confirm if this is fixed?