nsBufferedAudioStream::GetPositionInFrames reports bogus values in nightly Win32 builds

VERIFIED FIXED in Firefox 15

Status

()

Core
Audio/Video
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
mozilla17
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox15 verified, firefox16 verified)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Open the linked testcase, click "4410", note that p is some insanely large value instead of something between 0 and 4410.

I can reproduce this in official nightly builds, but not my own optimized or debug builds (with VC++ 2008 or 2010).  Presumably the major difference between my VC++ 2010 optimized builds the nightlies is that mine lack PGO.

It looks like the problem is inside GetPositionInFrames():

  return GetPositionInFramesUnlocked();
016AC6C5  push        esi  
016AC6C6  call        nsBufferedAudioStream::GetPositionInFramesUnlocked (16AC638h) 
016AC6CB  push        eax  
016AC6CC  push        edx  
016AC6CD  push        dword ptr [esi+18h] 
016AC6D0  call        dword ptr [__imp__PR_Unlock (1B93320h)] 
016AC6D6  pop         ecx  
016AC6D7  pop         eax  
016AC6D8  pop         edx  
016AC6D9  pop         esi  
}
016AC6DA  ret              

Note that eax and edx are saved to the stack before calling PR_Unlock, but they're restored in the wrong order, resulting in the high and low part of the result being exchanged.
(Assignee)

Comment 1

5 years ago
Disassembled versions of GetPositionInFramesUnlocked() and the caller (nsHTMLAudioElement::MozCurrentSampleOffset()) in local optimized and nightly PGO builds are more or less identical, but my local optimized build has quite a different GetPositionInFrames():

  return GetPositionInFramesUnlocked();
0150BB0B  mov         ecx,esi 
0150BB0D  call        nsBufferedAudioStream::GetPositionInFramesUnlocked (150B7EAh) 
0150BB12  mov         esi,eax 
0150BB14  mov         eax,dword ptr [mon] 
0150BB17  push        dword ptr [eax] 
0150BB19  mov         edi,edx 
0150BB1B  call        dword ptr [__imp__PR_Unlock (18D8214h)] 
0150BB21  pop         ecx  
0150BB22  mov         edx,edi 
0150BB24  pop         edi  
0150BB25  mov         eax,esi 
0150BB27  pop         esi  
}

Note that it saves eax and edx into esi and edi rather than using the stack, and restores them correctly after PR_Unlock.
(Assignee)

Updated

5 years ago
Summary: nsBufferedAudioStream::GetPositionInFrames reports bogus values in nightly builds → nsBufferedAudioStream::GetPositionInFrames reports bogus values in nightly Win32 builds
(Assignee)

Comment 2

5 years ago
Tightened up some type handling, let's see if this helps: https://tbpl.mozilla.org/?tree=Try&rev=b382e3f5c627
(Assignee)

Comment 3

5 years ago
Created attachment 642726 [details] [diff] [review]
patch v0

This is the patch I pushed.  I'll check the resulting binary from the try build before requesting review.
(Assignee)

Comment 4

5 years ago
Comment on attachment 642726 [details] [diff] [review]
patch v0

This patch doesn't work.
Attachment #642726 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
Let's try disabling PGO for that function: https://tbpl.mozilla.org/?tree=Try&rev=04115470bdc1
(Assignee)

Comment 6

5 years ago
Created attachment 647401 [details] [diff] [review]
patch v1

Disabling optimization of that functions does the trick.  Kind of gross, but I don't have a better fix on hand.
Attachment #647401 - Flags: review?(cpearce)
Attachment #647401 - Flags: review?(cpearce) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/376aa1851c9a

Now that there's a fix in hand, we'll want this on the other branches that cubeb (bug 623444) landed on.
Target Milestone: --- → mozilla17
(Assignee)

Comment 8

5 years ago
Comment on attachment 647401 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 623444
User impact if declined: mozCurrentSampleOffset broken in Win32 PGO builds
Testing completed (on m-c, etc.): completed locally
Risk to taking this patch (and alternatives if risky): very low, disables optimization for a simple getter function
String or UUID changes made by this patch:
Attachment #647401 - Flags: approval-mozilla-beta?
Attachment #647401 - Flags: approval-mozilla-aurora?

Comment 9

5 years ago
Comment on attachment 647401 [details] [diff] [review]
patch v1

>+  PRInt64 position = mAudioStream->GetPositionInFrames();
>+  if (position < 0) {
>+    *aRetVal = 0;
>+  } else {
>+    *aRetVal = mAudioStream->GetPositionInFrames() * mChannels;
[Why not position * mChannels?]
(Assignee)

Comment 10

5 years ago
Created attachment 647470 [details] [diff] [review]
followup v0

(In reply to neil@parkwaycc.co.uk from comment #9)
> [Why not position * mChannels?]

It should be, I just missed replacing the second call.  Thanks!
Attachment #647470 - Flags: review?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/376aa1851c9a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Attachment #647470 - Flags: review?(cpearce) → review+
Comment on attachment 647401 [details] [diff] [review]
patch v1

low risk, approving.
Attachment #647401 - Flags: approval-mozilla-beta?
Attachment #647401 - Flags: approval-mozilla-beta+
Attachment #647401 - Flags: approval-mozilla-aurora?
Attachment #647401 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 13

5 years ago
Landed the followup patch: http://hg.mozilla.org/integration/mozilla-inbound/rev/ae767e8de495

I'll roll that tiny change into the first patch when landing on aurora and beta.
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/43bf08945eaf
http://hg.mozilla.org/releases/mozilla-beta/rev/ca388f2eea7d
status-firefox15: --- → fixed
status-firefox16: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/ae767e8de495
(In reply to Matthew Gregan [:kinetik] from comment #0)
> Open the linked testcase, click "4410", note that p is some insanely large
> value instead of something between 0 and 4410.

I might not understand the issue. First time I click "4410" or any other buttons, p=0. Second time I click "4410", p=5364414152704.
Tested on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b3

Isn't this landed into FF 15b3 ?
I got different results on Nightly 17.0a1 (2012-08-02), p=812 when clicked twice on "4410".
(Assignee)

Comment 17

5 years ago
(In reply to Paul Silaghi [QA] from comment #16)
> I might not understand the issue. First time I click "4410" or any other
> buttons, p=0. Second time I click "4410", p=5364414152704.
> Tested on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101
> Firefox/15.0b3

That's the broken behaviour, any time p is larger than rw we're hitting this bug.

> I got different results on Nightly 17.0a1 (2012-08-02), p=812 when clicked
> twice on "4410".

That's the correct behaviour.

> Isn't this landed into FF 15b3 ?

It looks like it missed the 15b3 build (it was tagged at Tue Jul 31 14:54:57 2012 -0700 and I landed at Tue Jul 31 18:30:51 2012 -0700).

I've just confirmed the patch works as expected in the latest Aurora build.  I guess we'll need to wait for 15b4 to confirm it in 15.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b4
status-firefox15: fixed → verified
Verified fixed on FF Aurora 16 based on comment 17.
Status: RESOLVED → VERIFIED
status-firefox16: fixed → verified
You need to log in before you can comment on or make changes to this bug.