Intermittent test_reset_src.html | videoHeight should be zero when there is no video. - got 300, expected 0 | videoWidth should be zero when there is no video. - got 400, expected 0

RESOLVED FIXED in Firefox 29, Firefox OS v1.4

Status

()

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: RyanVM, Assigned: jwwang)

Tracking

({intermittent-failure})

Trunk
mozilla31
ARM
Android
intermittent-failure
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed, firefox31 fixed, firefox-esr24 unaffected, b2g-v1.3 unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
https://tbpl.mozilla.org/php/getParsedLog.php?id=31319325&tree=Mozilla-Inbound

Android 4.0 Panda mozilla-inbound opt test mochitest-3 on 2013-12-02 07:37:13 PST for push 04ab309c684a
slave: panda-0086

07:59:32     INFO -  18555 INFO TEST-START | /tests/content/media/test/test_reset_src.html
07:59:32     INFO -  18556 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoHeight.
07:59:32     INFO -  18557 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoWidth.
07:59:32     INFO -  18558 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozPaintedFrames should be positive, is 32.
07:59:32     INFO -  18559 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozFrameDelay should be positive or zero, is 0.011788694.
07:59:32     INFO -  18560 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoHeight.
07:59:32     INFO -  18561 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | We should have a videoWidth.
07:59:32     INFO -  18562 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozPaintedFrames should be positive, is 38.
07:59:32     INFO -  18563 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozFrameDelay should be positive or zero, is 0.020652729.
07:59:32     INFO -  18564 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_reset_src.html | videoHeight should be zero when there is no video. - got 300, expected 0
07:59:32     INFO -  18565 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_reset_src.html | videoWidth should be zero when there is no video. - got 400, expected 0
07:59:32     INFO -  18566 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozPaintedFrames should be zero when there is no video.
07:59:32     INFO -  18567 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | mozFrameDelay should be zero when there is no video.
07:59:32     INFO -  18568 INFO TEST-PASS | /tests/content/media/test/test_reset_src.html | Trying to draw to a canvas should throw, since we don't have a frame anymore
07:59:32     INFO -  18569 INFO TEST-INFO | MEMORY STAT vsize after test: 878800896
07:59:32     INFO -  18570 INFO TEST-INFO | MEMORY STAT residentFast after test: 216948736
07:59:32     INFO -  18571 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 38739972
07:59:32     INFO -  18572 INFO TEST-END | /tests/content/media/test/test_reset_src.html | finished in 3690ms
Assignee: nobody → edwin
Status: NEW → ASSIGNED
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 58

5 years ago
Edwin, I don't suppose you were going to circle back around to this at some point?
Flags: needinfo?(edwin)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #58)
> Edwin, I don't suppose you were going to circle back around to this at some
> point?

I have quite a lot on my plate at the moment; I think jwwang is looking at media oranges at the moment, though.
Flags: needinfo?(edwin)
Comment hidden (Treeherder Robot)
JW, are you able to take this?
Flags: needinfo?(jwwang)
(Assignee)

Comment 66

5 years ago
sure.
Assignee: edwin → jwwang
Flags: needinfo?(jwwang)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 74

4 years ago
root cause:
1. 'src' is changed from a video to an audio file in the test
2. HTMLMediaElement::ResetState() is called in HTMLMediaElement::Load() to reset |mMediaSize| to (-1,-1).
3. pending MediaDecoder::PlaybackPositionChanged() calls MediaDecoder::Invalidate() which indirectly calls HTMLMediaElement::UpdateMediaSize() to overwrite |mMediaSize| and results in staled videoWidth/videoHeight.

fix:
Call VideoFrameContainer::ForgetElement() to prevent staled events from reach the media element.
(Assignee)

Comment 75

4 years ago
Created attachment 8397557 [details] [diff] [review]
part1_fix_v1.patch

see comment 74 for what this patch does.
Attachment #8397557 - Flags: review?(cpearce)
Comment on attachment 8397557 [details] [diff] [review]
part1_fix_v1.patch

Review of attachment 8397557 [details] [diff] [review]:
-----------------------------------------------------------------

ImageContainer/VideoFrameContainer is roc's territory, passing review to him...
Attachment #8397557 - Flags: review?(cpearce) → review?(roc)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 81

4 years ago
(In reply to JW Wang[:jwwang] from comment #74)
Sorry, I got the root cause wrong. Since |mShuttingDown| is true (HTMLMediaElement::Load => HTMLMediaElement::AbortExistingLoads => HTMLMediaElement::ShutdownDecoder => MediaDecoder::Shutdown) in MediaDecoder::PlaybackPositionChanged(), MediaDecoder::Invalidate won't be called.

The correct root cause is as follows:
1. 'src' is changed from a video to an audio file in the test
2. VideoFrameContainer::Reset => |mIntrinsicSize| = (-1,-1)
3. MediaDecoderStateMachine::RenderVideoFrame => VideoFrameContainer::SetCurrentFrame => |mIntrinsicSizeChanged| = true
4. MediaDecoder::MetadataLoaded => MediaDecoder::Invalidate => VideoFrameContainer::InvalidateWithFlags => HTMLMediaElement::UpdateMediaSize

For calling RenderVideoFrame() is wrapping in a ReentrantMonitorAutoExit monitor, it allows main thread to cut in and change the decoder state during the execution of RenderVideoFrame(). It seems to me that wherever ReentrantMonitorAutoExit is used in MediaDecoderStateMachine, we have to check the decoder state again to prevent staled changes propagated to the main thread. (Bug 760770 might be related)

Fix:
Call VideoFrameContainer::ForgetElement() to prevent staled events from reaching the media element. Also delete VideoFrameContainer more aggressively to prevent it from being shared between multiple decoders or DOMMediaStreams.
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 88

4 years ago
Hi Roc,
Can you help review the patch? The cause and fix are explained in Comment 81. Thanks.
Flags: needinfo?(roc)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 90

4 years ago
try is green: https://tbpl.mozilla.org/?tree=Try&rev=339f38804141
Flags: needinfo?(roc)
Keywords: checkin-needed
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Reporter)

Comment 96

4 years ago
https://hg.mozilla.org/mozilla-central/rev/b16e2cb162f5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
(Reporter)

Comment 97

4 years ago
Please request Aurora/Beta approval on this when you get a chance :)
status-b2g-v1.3: --- → unaffected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → fixed
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → fixed
status-firefox-esr24: --- → unaffected
Flags: needinfo?(jwwang)
(Assignee)

Comment 98

4 years ago
I am on a PTO until 4/9. Will get it done once I am back. :)
Flags: needinfo?(jwwang)

Comment 99

4 years ago
The following changeset is now in Firefox Nightly:

> b16e2cb162f5 Bug 945475 - clear |mVideoFrameContainer| to stop staled callbacks which give incorrect videoWidth/videoHeight. r=roc

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
(Assignee)

Comment 104

4 years ago
Created attachment 8405132 [details] [diff] [review]
part1_fix_aurora.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: user may get incorrect videoWidth/videoHeight after changing 'src' attribute
Testing completed (on m-c, etc.): try: https://tbpl.mozilla.org/?tree=Try&rev=da1ed7aa4620
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8405132 - Flags: review+
Attachment #8405132 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 105

4 years ago
Created attachment 8405136 [details] [diff] [review]
part1_fix_beta.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: user may get incorrect videoWidth/videoHeight after changing 'src' attribute
Testing completed (on m-c, etc.): try: https://tbpl.mozilla.org/?tree=Try&rev=c37658a90ee3
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8405136 - Flags: review+
Attachment #8405136 - Flags: approval-mozilla-beta?
Attachment #8405132 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8405136 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Reporter)

Comment 106

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/56eb28be7cb5
https://hg.mozilla.org/releases/mozilla-beta/rev/eaf92a872145
status-b2g-v1.4: affected → fixed
status-firefox29: affected → fixed
status-firefox30: affected → fixed
You need to log in before you can comment on or make changes to this bug.