Closed Bug 677122 Opened 13 years ago Closed 6 years ago

Changing media fragment portion of URL in video documents doesn't update start/end times

Categories

(Core :: Audio/Video: Playback, defect)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INCOMPLETE

People

(Reporter: cajbir, Assigned: scott.downe)

References

Details

Attachments

(1 file, 10 obsolete files)

Update start/end times of mediafragment URL on hashchanges
6.33 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
When navigating directly to a video, changing the media fragment portion of the URL doesn't change the start/end times of the video. You need to refresh the page to get this to happen.

Steps to reproduce:

1) Open http://www.bluishcoder.co.nz/mediafrag/AudioAPI.webm#t=50,100
2) Change the fragment portion to be "#t=30.100" and press enter

Expected Results:

1) Start and End fragment of the video would be the new position of 30 and 100 respectively.

Actual Results:

1) The video does not change the start/end fragments. They stay at 50 and 100.
Depends on: 648595
Summary: Changing media fragment portion of URL in video documents doesn't update play position → Changing media fragment portion of URL in video documents doesn't update start/end times
Assignee: nobody → scott.downe
Status: NEW → ASSIGNED
Attached patch Proposed solution (obsolete) — — Splinter Review 
I got this working with a reload of the document. Not currently able to seek the video as needed without having to reload, may need help with that :)

It was interesting to not that a media fragment string "#t=20,100" would go into the anchor handling code, this chokes that off.

By choking that off, it will reload the page because it has nothing else to do, which in turn will reload the video and thus call ProcessMediaFragment.

My first patch, so any comments on simple process or style related things is appreciated.
Attachment #592407 - Flags: feedback?(david.humphrey)
Attachment #592407 - Flags: feedback?(chris.double)
It just hit me, this will not work.

Not only is #t a valid anchor, #t=60,50 is also a valid anchor, so checking against the string is a silly idea.
I also fixed a small typo in a comment I modified, and moved a variable into the only scope it is used in.
Attachment #592407 - Attachment is obsolete: true
Attachment #592407 - Flags: feedback?(david.humphrey)
Attachment #592407 - Flags: feedback?(chris.double)
Attachment #595603 - Flags: review?
Attachment #595603 - Flags: review? → review?(bzbarsky)
Comment on attachment 595603 [details] [diff] [review]
Don't do a short-circuited load on synthetic documents

This would violate step 7 of http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigating-across-documents and the http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#scroll-to-fragid it links to.

What we need to do here is to "perform some other action, such that the indicated part of the document is brought to the user's attention" without changing the document object.

Most simply, would adding a script to the <head> (allowed per http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#read-media ) that watches for hashchange events and adjusts things as needed work?
Attachment #595603 - Flags: review?(bzbarsky) → review-
Thanks for the review!

I have a patch with an attempt at your approach.

There are two things I really like about this approach, one being it is in JavaScript which is really awesome! The other being how modular it is, so it is pretty safe that nothing can break because of this.

One thing I don't like is how it is reproducing the code for media fragments twice. There is already c++ code to parse this, and things would be more maintainable if we could simply tap into that at some point. Ideally call the parseMediaFragment code via the hashchange js event, but I doubt the js can call the mediaFragmentParser. That is my thinking anyway.

I also had problems finding a good place to put the mediaFragment.js file. I am going to continue to look into that, but I feel putting this out there now as is and getting some feedback is crucial.
Attachment #595603 - Attachment is obsolete: true
Attachment #600490 - Flags: review?(bzbarsky)
Hrm.  Duplicating the parser in the JS isn't quite what I had in mind....

Does just changing media.src to be the new document URI not in the hashchange listener do the right thing by any chance?

If it doesn't, it's worth talking to some of the media folks about good ways to do this.  Ideally we could just pass off the work to all the existing C++ code.

As far as where to put the file... one option is to use an inline script altogether.  Esp. if the script ends up short enough.  ;)

Speaking of which, you could do:

  var media = document.querySelector("audio,video");

to get the media element.
I actually don't see a src on the video element in a media doecument, but, adding one, and changing it does give us the desired functionality.

I did have it inline, but I explored it as a file too. It felt too long the way it was. If we can get it as short and simple as:

"document.querySelector("audio,video")[0].src = newSrc;" inside the hashchange then inline would be perfect.

I will give this a go. Thanks again.
> I actually don't see a src on the video element in a media doecument

It has one internally, just not in an attribute.

> "document.querySelector("audio,video")[0].src = newSrc;"

Yeah, if that works, that sounds good.  ;)
Awesome, starting to take shape :)
Attachment #600490 - Attachment is obsolete: true
Attachment #600490 - Flags: review?(bzbarsky)
Attachment #600607 - Flags: review?(bzbarsky)
Comment on attachment 600607 [details] [diff] [review]
reload src element on hashchange for media documents

>+++ b/content/html/document/src/MediaDocument.cpp
>+nsresult

No one checks the return value... just return void?

>+MediaDocument::InsertMediaFragmentScript()
>+  nsCOMPtr<nsINodeInfo> nodeInfo;

Please declare it right next to the code that assigns it?

>+  nsRefPtr<nsGenericHTMLElement> script = NS_NewHTMLScriptElement(nodeInfo.forget());
>+  NS_ENSURE_TRUE(script, NS_ERROR_OUT_OF_MEMORY);

NS_NewHTML*Element never return null, so no need to check the return value.  Which is good, because this code would leak on null return if it could happen!

>+  script->SetInnerHTML(NS_LITERAL_STRING("window.addEventListener(\"hashchange\",function(e){document.querySelector(\"audio,video\").src=e.newURL;},false);"));

SetTextContent, please.

If you use single quotes in your JS, you don't have to escape them.

>+  script->SetAttr(kNameSpaceID_None, nsGkAtoms::type, NS_LITERAL_STRING("text/javascript"), true);

No need for that; scripts with no type set default to JavaScript.

> MediaDocument::SetScriptGlobalObject(nsIScriptGlobalObject* aGlobalObject)
>+  if (aGlobalObject && !nsContentUtils::IsChildOfSameType(this)) {

The IsChildOfSameType check is wrong.  It would make changes to the hash not work in subframes.  When you write tests for this, make sure to test that?

The check was there for the stylesheet because we don't want our "full page media" styling in subframes, in fact.

r=me with those nits fixed.  Thanks for doing this!
Attachment #600607 - Flags: review?(bzbarsky) → review+
Sorry for the delay; I had problems with NS_ENSURE_TRUE returning nsresult, took a break, then got some help. Was exactly what I needed.

I replaced the NS_ENSURE_TRUE with an if statement and a void return. Not 100% confident we event need it there.

Thanks for the review.
Attachment #600607 - Attachment is obsolete: true
Attachment #604410 - Flags: review?(bzbarsky)
Comment on attachment 604410 [details] [diff] [review]
reload media document on media fragment hash change

r=me

Would be good to add a test if you can figure out how, of course.  And please add a commit message and From line to the patch before requesting checkin!
Attachment #604410 - Flags: review?(bzbarsky) → review+
Added a mochitest.

Not quite sure where to go from here.

Is there a two part review process? Should I be passing this on to someone else?

Either way, I think Boris should take one last look, sanity check on the test I added :)

Thanks!
Attachment #604410 - Attachment is obsolete: true
Attachment #609185 - Flags: review?(bzbarsky)
Comment on attachment 609185 [details] [diff] [review]
reload media document on media fragment hash change

The commit comment should actually say what's changing... there's no reloading happening here.

r=me with that fixed.  Thanks for the test!

What happens next, after you fix the checkin comment, is you add the checkin-needed keyword to this bug.  There _is_ a 2-part review process, but only for changes that change APIs and whatnot; for most things a single review is enough.
Attachment #609185 - Flags: review?(bzbarsky) → review+

        Attachment #609185 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
  
Comment on attachment 609568 [details] [diff] [review]
Update start/end times of mediafragment URL on hashchanges

Carrying forward r+ from bz.

        Attachment #609568 -
        Flags: review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/389eee1cce65
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla14

    
    

    
  
Sample log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10419024&tree=Mozilla-Inbound

PROCESS-CRASH | /tests/dom/tests/mochitest/dom-level2-html/test_HTMLFrameElement01.html | application crashed (minidump found)
Crash dump filename: /tmp/tmps60MVV/minidumps/43051c7b-f3d7-38b2-36a4d674-1e9059ba.dmp
Operating system: Linux
                  0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  SIGSEGV
Crash address: 0x0

Thread 0 (crashed)
 0  libxul.so!nsINode::AppendChildTo [nsINode.h : 579 + 0x3]
    eip = 0x0112f564   esp = 0xbf9a6164   ebp = 0x00000000   ebx = 0x0227cc60
    esi = 0x00000000   edi = 0xbf9a61b8   eax = 0x00000000   ecx = 0x9b406d84
    edx = 0x00000000   efl = 0x00010292
    Found by: given as instruction pointer in context
 1  libxul.so!mozilla::dom::MediaDocument::InsertMediaFragmentScript [MediaDocument.cpp : 382 + 0xe]
    eip = 0x014318ef   esp = 0xbf9a6180   ebp = 0x9b406c00   ebx = 0x0227cc60
    esi = 0xbf9a61bc   edi = 0xbf9a61b8
    Found by: call frame info
 2  libxul.so!mozilla::dom::ImageDocument::SetScriptGlobalObject [ImageDocument.cpp : 366 + 0x8]
    eip = 0x0142f5d2   esp = 0xbf9a61e0   ebp = 0x9b406c00   ebx = 0x0227cc60
    esi = 0x9b406c00   edi = 0x9d61f2b8
    Found by: call frame info
 3  libxul.so!nsGlobalWindow::SetNewDocument [nsGlobalWindow.cpp : 2301 + 0x16]
    eip = 0x014c6991   esp = 0xbf9a6270   ebp = 0x9b406c00   ebx = 0x0227cc60
    esi = 0x9d61c180   edi = 0xa0a22540
    Found by: call frame info
 4  libxul.so!DocumentViewerImpl::InitInternal [nsDocumentViewer.cpp : 980 + 0x14]
    eip = 0x01164679   esp = 0xbf9a63d0   ebp = 0x99b44050   ebx = 0x0227cc60
    esi = 0x9b95a418   edi = 0x00000001
    Found by: call frame info
 5  libxul.so!DocumentViewerImpl::Init [nsDocumentViewer.cpp : 723 + 0x18]
    eip = 0x01164c5a   esp = 0xbf9a6470   ebp = 0x9b95a400   ebx = 0x0227cc60
    esi = 0x9b95a400   edi = 0x80004005
    Found by: call frame info
 6  libxul.so!nsDocShell::SetupNewViewer [nsDocShell.cpp : 7714 + 0x17]
    eip = 0x017b77ed   esp = 0xbf9a64a0   ebp = 0x9b95a400   ebx = 0x0227cc60
    esi = 0x9b95a400   edi = 0x80004005
    Found by: call frame info
 7  libxul.so!nsDocShell::Embed [nsDocShell.cpp : 5820 + 0xb]
    eip = 0x017bde0a   esp = 0xbf9a66b0   ebp = 0x80004005   ebx = 0x0227cc60
    esi = 0x9b95a400   edi = 0x99b44050
    Found by: call frame info
 8  libxul.so!nsDocShell::CreateContentViewer [nsDocShell.cpp : 7500 + 0x1a]
    eip = 0x017c0a93   esp = 0xbf9a66e0   ebp = 0x80004005   ebx = 0x0227cc60
    esi = 0x9b95a400   edi = 0x9bb90830
    Found by: call frame info
 9  libxul.so!nsDSURIContentListener::DoContent [nsDSURIContentListener.cpp : 164 + 0x14]
    eip = 0x017c5b92   esp = 0xbf9a6760   ebp = 0x9bb90830   ebx = 0x0227cc60

    
    

    
  
Er, yes.  Image documents, unlike video documents, don't create the synthetic document until their SetScriptGlobalObject is called...

So we probably need to either null-check the GetHeadElement(), or just put this code into VideoDocument (where it would make more sense anyway), not MediaDocument.

    
    

    
  
If we put it into videoDocument, would it still work for audio?

Also, I am requesting try server https://bugzilla.mozilla.org/show_bug.cgi?id=739873 to help put out that fire :)

    
    

    
  
> If we put it into videoDocument, would it still work for audio?

Yes.  VideoDocument is used for both audio and video.  It should perhaps be named MediaDocument, but there's already a MediaDocument (used for audio, video, images, and plug-is).

    
    

    
  
Perfect. I only had it in mediaDocument because I thought that meant video+audio.

I will give that a try.

    
    

    
  


  
Everything is working with it on the videoDocument.

I am still waiting on try server access, but I have the updated patch, and I'll see if I can get someone to run it through for me.

        Attachment #609568 -
        Attachment is obsolete: true

        Attachment #613071 -
        Flags: review?(bzbarsky)

    
    

    
  


  
A small update to the test to get consistent results. This was exposed by running it through the try server.

These are the results now from try: https://tbpl.mozilla.org/?tree=Try&rev=1b63bb1867e8

Are the oranges acceptable and known? Not sure how to make sense of all this, but the old failures are gone.

        Attachment #613071 -
        Attachment is obsolete: true

        Attachment #613071 -
        Flags: review?(bzbarsky)

        Attachment #613219 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 613219 [details] [diff] [review]
Moved media fragment hash change code off mediaDocument and onto videoDocument

r=me

        Attachment #613219 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/14715fd6b81c

Had to un-bitrot VideoDocument.cpp a bit. Please check it to make sure I didn't screw anything up.
Keywords: checkin-needed

    
    

    
  
Sorry, I had to back this out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f03124b00990

because the test was failing intermittently but frequently:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10810377&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=10810913&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=10810466&tree=Mozilla-Inbound

Someone will need to figure out and fix the test failures before this can re-land.  Please let us know if you need any help with that!

    
    

    
  
There are two issues here.

One is the "Expected number of tests run. Expected: 5 Actual: 1", where the video I test against goes into what looks to be streaming mode. I can only reproduce this inside the mochi test. It is pretty elusive, but once you have a failing case, you can refresh the mochi test and it will continue to fail.

The other one, the "Assertion failure: !IsNull() (Cannot compute with a null value), at ../../dist/include/mozilla/TimeStamp.h:245" I have not seen yet. I have a breakpoint there, and hope to catch it, but... this one stumps me. I have no idea where that is being called from.

I hope, and suspect this is because of my inexperience in writing mochitests, and it is my test itself that is broken.

    
    

    
  
Oh, and the "Assertion failure: !IsNull() (Cannot compute with a null value), at ../../dist/include/mozilla/TimeStamp.h:245" passes my first test, so it is crashing after that test, but before the next.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/14715fd6b81c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
  
Backed out: https://hg.mozilla.org/mozilla-central/rev/f03124b00990
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
Does the log not have a stack for the assertion failure?

    
    

    
  
Yeah, there is a stack.

I am attempting to understand it now.

Thread 25 (crashed)
 0  linux-gate.so + 0x424
    eip = 0x00d81424   esp = 0xa34fdfe0   ebp = 0xa34fdff8   ebx = 0x0000088c
    esi = 0x00c59844   edi = 0x00c7cff4   eax = 0x00000000   ecx = 0x00000930
    edx = 0x00000006   efl = 0x00000202
    Found by: given as instruction pointer in context
 1  firefox-bin!MOZ_Crash [Assertions.cpp : 90 + 0x6]
    eip = 0x0804b2ad   esp = 0xa34fe000   ebp = 0xa34fe018
    Found by: previous frame's frame pointer
 2  firefox-bin!MOZ_Assert [Assertions.cpp : 104 + 0x4]
    eip = 0x0804b2f4   esp = 0xa34fe020   ebp = 0xa34fe038   ebx = 0x0804ca84
    Found by: call frame info
 3  libxul.so!mozilla::TimeStamp::operator- [TimeStamp.h : 245 + 0x1f]
    eip = 0x0149f12e   esp = 0xa34fe040   ebp = 0xa34fe068   ebx = 0x02bf728c
    esi = 0x0c0fd9e8
    Found by: call frame info
 4  libxul.so!nsBuiltinDecoderStateMachine::AdvanceFrame [nsBuiltinDecoderStateMachine.cpp : 1999 + 0x15]
    eip = 0x01a84680   esp = 0xa34fe070   ebp = 0xa34fe148   ebx = 0x02bf728c
    esi = 0x0c0fd9b8   edi = 0xa34fe0f8
    Found by: call frame info

mValue of the timestamp is null when we attempt a:

    TimeStamp presTime = mPlayStartTime - UsecsToDuration(mPlayDuration) +
                          UsecsToDuration(currentFrame->mTime - mStartTime);

It is the currentFrame->mTime - mStartTime that causes it. It is line 1999 nsBuiltinDecoderStateMachine.cpp.

This goes into the minus operator, which fails because mValue is null.

    
    

    
  
Sounds like either currentFrame->mTime or mStartTime never got initialized.  Which one of the to is it?

    
    

    
  
I am pretty sure it is currentFrame->mTime, based on what I now know about operator overloading.

The operator fails because the TimeStamp local mValue is null, which should be the left hand side of the operation, thus the currentFrame->mTime.

    
    

    
  
Interesting, there is a if (currentFrame) { right before the call that fails.

Based on that logic, I wonder if there would be any repercussions to also checking that !currentFrame.mTime.IsNull()

So we only do the check if there is a currentFrame and the frame's time is not null.

We don't want something uninitialized running around, that never will be initialized. Just... thoughts.

    
    

    
  
I am stumped on this. Matt Brubeck, I am going to take you up on the help offer :)

I am convinced looking at the stack traces that the fails are due to the mochitest itself.

But, I am having a hard time proving that.

Help with creating the mochitest would be great, or at the least a review of what I have, to see if there are better ways to test what I am testing.

The "Expected number of tests run. Expected: 5 Actual: 1" fail is due to the video going into what looks to be streaming, where the playhead is always at the end of the timeline, and you can only seek to a time that has already been played, making media fragment tests on a videoDocument impossible when it does this. It only does this sometimes with my test, though.

I can reproduce what I mean all the time with this reduced test case:

<!DOCTYPE HTML>
<html>
<head>
<script type="text/javascript">
    var url = "../../../media/test/seek.ogv";
    window.open(url);
</script>
</head>
<body>
</body>
</html>

This has to be run as a mochitest to cause the streaming mode, but it happens every time. I don't see another way to test functionality of videoDocuments other than iframes of window.open.

The Assertion failure issue stumps me more, but I am also pretty sure it is either a bug in my test, or a deeper bug I am exposing, but not causing. What makes me think this is in the stack trace where it failed, my first test passes, which I would suspect would not even happen if my patch was introducing the bug.

I am going to try and find some existing tests for the videoDocument, see what those look like. maybe a mochitest is the wrong approach?

    
    

    
  
Changing the url in the above case to a remote one stops the streaming. It has something to do with the url not working while local in a mochitest.

    
    

    
  
A mochitest local url is http://mochi.test:8888/tests/content/media/test/seek.ogv

It is the mochi.test:8888 that I am interested in. It is the only difference between this, and running it outside a mochitest, or remote urls.

    
    

    
  
Files at http://mochi.test are served by a simple web server written in JavaScript:
https://mxr.mozilla.org/mozilla-central/source/testing/mochitest/server.js

Is it possible that limitations or performance of this HTTP server are interfering with the test?

    
    

    
  
I am going to reduce my test to what I think causes these problems, and run that in try without the rest of my patch. See if it still fails, so we'll know if it is limitations or my patch.

I will also take a look at that server.

    
    

    
  
I've reduced my test to what is causing the problem.

I have a test that I would expect to pass, that will fail every time, on a clean branch.

https://tbpl.mozilla.org/?tree=Try&rev=2c47a9772f56

https://hg.mozilla.org/try/rev/2c47a9772f56

This is an example of the streaming issue I was talking about.

My test tries to trigger a mediafragment seek, and listens for it. It never happens, instead plays to the end and I listen for that, to fail the test.

I believe this is in mochitest/server.js, and I am moving my attention there.

Should this be filed in a new ticket? or just fix it here? Am I crazy to think this test should pass?

    
    

    
  
If is fails if the file is served by the mochitest framework, but not from a 'normal' http server, then it's better to open a separate bug for that.

    
    

    
  
Chris Double is on holiday right now, he'll be back in a couple of weeks. If you can wait that long, he's the best person to help here.

    
    

    
  
What's the possibility of landing this without tests? I don't think it is testable at this point.

In comment 12 it was mentioned it would be good if I could figure out how to write a test for this... I cannot.

I filed a bug on mochitests: https://bugzilla.mozilla.org/show_bug.cgi?id=747584

Without my tests this is the try result I get: https://tbpl.mozilla.org/?tree=Try&rev=14bb8473d1a1

There are a few errors in andoid that I don't know what to make of.

At this point, I am up for any ideas or options, as I am out of them.

    
    

    
  
If you run the parts of the test async instead of sync, does that help?

    
    

    
  
I'm not sure what you mean by sync and async. I am running the tests inside event listeners. Is that what you mean by async?

Is this a mochitest specific thing? Like an async mode?

I did actually just have an idea. We don't test the functionality directly, but test the existence and content of the script element I inserted. That is an option that is better than nothing.

I am curious as to what you mean by asynch, though.

    
    

    
  
I meant replacing the places where you call next() with:

  // Give whatever the media code is doing right now a chance to complete
  // before we seek.
  SimpleTest.executeSoon(next);

    
    

    
  
Cool. I am trying that solution: https://tbpl.mozilla.org/?tree=Try&rev=873fac3250a4

*fingers crossed*

what does executeSoon do anyway? Does it wait until everything has stopped?

Boris, if this works, you would be a hero!

Our efforts are not for naught though, as I have filed two bugs while trying to track this down.

    
    

    
  
> what does executeSoon do anyway?

Posts an event to the event loop to call the function when the event fires.  The idea being to not do the seek until the event handler you're in right now has returned and the stack unwound.

If that fixes things, we may need another bug filed unless the two you filed cover things already, because it's not good when web content triggers asserts.  ;)

    
    

    
  
You could put the <iframe> and initial src directly into your HTML file instead of creating and inserting it via the DOM. That would sipmlify things a little bit.

I don't think your "seeked" event handler is quite right. The current time might have advanced past test.start by the time the "seeked" handler runs.

Are you expecting to get a "pause" *and* "ended" event when we reach the end of the first interval? If so, why are we expecting count to be 3 in "ended"? Seems to me it would only be 2.

    
    

    
  
I initially had the iframe in the HTML, but that is one of the leading causes of the streaming issues. That makes it happen every time, as opposed to some times.

My example in the bug I filed has the iframe in the html.

Now, it might solve my problem if I put the src also in the html, but then I may miss my load event. The load event might fire before I can get the reference to the iframe and setup an event. Ideas for that?

You're most likely right on the seeked bit. I will change the time comparisons to greater than or equal to, see if that helps. That way if it advances a bit past, it won't be a problem.

The first test is the range of 1-2, second being 3-4. 4 is the end of the video.

The 3-4 range actually triggers an end, and not a pause, where as the 1-2 triggers a pause.

This also means if the functionality breaks, and the seeked and pause don't happen, the end still happens, and the tests fail, instead of timing out. I would take improvements on this, if you have some, though.

Thanks for the feedback.

    
    

    
  
> Ideas for that?

  <script>
    function foo() {
      // do stuff
    }
  </script>
  <iframe onload="foo()" src="whatever"></iframe>

    
    

    
  
Sweet, running those suggestions now: https://tbpl.mozilla.org/?tree=Try&rev=5a3f409f1844

I have good feelings about it.

The other bug I filed is here: https://bugzilla.mozilla.org/show_bug.cgi?id=745035

I was attempting to reproduce the assertion error, and came up with the above.

The assertion crash stacks, a reminder, for reference, and because we don't want to lose them:

https://tbpl.mozilla.org/php/getParsedLog.php?id=10810377&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=10810466&tree=Mozilla-Inbound

Not convinced yet they are the same, but they may be related. I have yet to see it myself though, other than the above stacks. There is definitely something weird going on with the loading and order of mediadocument mediafragments and iframes. Those stacks are still all we have from that.

    
    

    
  
The SimpleTest.executeSoon(next) doesn't quite do it, still got a seek error.

https://tbpl.mozilla.org/php/getParsedLog.php?id=11086360&tree=Try

That tells me the end was fired, and no other tests run, with seek and pause being skipped because it is streaming.

Still waiting on the iframe in html with onload="load()".

    
    

    
  


  
Test now uses both SimpleTest.executeSoon(next) and iframe is in the html with an onload.

I've run it through try multiple times and have yet to see a test fail or assertion error.

I want to try landing this again :)

        Attachment #613219 -
        Attachment is obsolete: true

        Attachment #617254 -
        Flags: review?(bzbarsky)

    
    

    
  


  
Ignore that last one.

I was testing something, so I had commented out code that was not intended in the diff.

Everything else should be the same.

        Attachment #617254 -
        Attachment is obsolete: true

        Attachment #617254 -
        Flags: review?(bzbarsky)

        Attachment #617265 -
        Flags: review?(bzbarsky)

    
    

    
  
Comment on attachment 617265 [details] [diff] [review]
Update start/end times of mediafragment URL on hashchanges

r=me but please make sure we have a bug tracking those test failures when the next() was synchronous!

        Attachment #617265 -
        Flags: review?(bzbarsky) → review+

    
    

    
  
Sorry, I backed this out again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6e8470cb96

because the test failed with:
test_bug677122.html | Expected number of tests run. Expected: 5 Actual: 1
https://tbpl.mozilla.org/php/getParsedLog.php?id=11175908&tree=Mozilla-Inbound
Target Milestone: mozilla15 → ---

    
    

    
  
Still not enough eh.

Question, is there a way to easily run this through try more than once?

I ask, because it doesn't fail often, and can have clean runs, only to have it fail later. After a fix, to be sure, I would like to run it multiple times.

    
    

    
  
You can restart jobs from the tbpl page (look for the + icon in the bottom right pane) or from the self-serve API.
Component: Audio/Video → Audio/Video: Playback

    
    

    
  
There's been no activity on this patch in 6 years, so I assume it won't apply cleanly now. If the issue is still present and you want to work on your fix more, please feel free to re-open.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 6 years ago
Resolution: --- → INCOMPLETE

    
    

    
  
I still experience this bug in 81.0.



          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: