Skip to content

Firefox bug 680321

February 7, 2012

Over the past 2 weeks I have been working primarily on bug 680321, which pertains to issue’s with the way Firefox handles the changing of the preload attribute.  I explain in a prior post the details surrounding the bug and some of my thinking towards getting my patch up for review.  Since then I have put another patch up for review, gotten some great feedback, and am working towards getting another working version up and running.

My first patch failed mainly because it was just wrong.  I created the desired functionality but in doing so I broke features that I shouldn’t have ( this happened due to the fact that I didn’t write any tests before I asked for feedback ).  What I broke was a case where the user changes the preload state, but does not change the media’s source.  If this happens firefox simply ignores the changed preload state as we are requesting to change the buffering algorithm on a video that is already buffering.  My first fix was to simply allow this to happen, which fulfilled my use case but obviously broke what I just outlined.  I quickly got some feedback saying that my approach was wrong and I got a few pointers in the right direction from Chris Pearce.

Chris gave me the following advice:

Can you look a the testcase attached here in the bug and add a similar test to content/media/test/test_preload_actions.html?

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ -889,5 @@
> nextAction = static_cast(preloadDefault);
> }
> }
>
> – if ((mBegun || mIsRunningSelectResource) && nextAction < mPreloadAction) {

So mBegun is true after we’ve chosen a resource and started to download and decode it. We don’t want to support reductions in the amount of buffering after decoding has begun at the current time; that should be the subject of another bug.

So instead of doing what you’re doing now, create a new boolean flag mHaveQueuedSelectResource, and use that in QueueSelectResourceTask instead of using mIsRunningSelectResource. This will prevent multiple calls to SelectResource being queued concurrently. Then call UpdatePreloadAction() in SelectResource, but set mIsRunningSelectResource to true in SelectResource() *after* calling UpdatePreloadAction().

Then the preload action will be updated before we start trying to load the resource, so if it’s changed after load() is called on the media element, but before the load’s synchronous section is run, the new load will reflect the changed preload state.

If that doesn’t make sense (or work!), let me know. :)

Seeing as Chris basically outlined a solution for me I would be dumb not to roll with it so that’s what I did.  After about two nights of writing tests and bring the code up to par I submitted it again for review.  The main problems that I got nailed for in review were not adding comments, which I guess I should of known would have been said considering how well commented the Firefox source code is.  I was also asked to write another test in order to prove the results of my fix.  The last thing I got nailed on was changing the use of mIsRunningSelectResource to mHaveQueuedResource.  One of the problems I encountered while writing the fix Chris suggested was that the fix broke the same previous case as my first fix did.  This was due to the fact that although we were loading a new resource and setting the preload state again, we were still running into the same issue as before because the resource selection algorithm still thought we were loading the original source. We ( currently ) had no way of explicitly stating that the source had changed so that it was ok to allow the preload state to change.  I managed to find a hacky workaround to this by leveraging the new variable that we created and I found a unique case where the boolean state of the flag accurately reflected that we were loading a new source.

Although I had a fix that satisfied both cases it was still not optimal, which is probably why I got another R-.  Currently I am waiting for Chris to come back to irc in order to have a conversation with him about the problem ( I think he is travelling at the moment ).  In the meantime I am going to be focusing my attention on a new bug I picked up which is working on Firefox’s JavaScript engine, SpiderMonkey.

Bug 722788 is a starter bug which explains there is a simple( for them ) optimization to be made when parsing JSON numbers from a string.  Instead of using double’s to do the parsing we can simply use unsigned integers to accomplish what we need.  This code is the most intense looking C code that I have seen to date, so I’m not gonna lie, I’m a bit intimidated.  I think I have what seems like a fix but I’m getting some odd results when I try and run the tests.  The next step here is to go into the #jsapi channel and ask how I am supposed to be running the tests and if there is any known fails I can expect to see.

I know this bug is a starter bug, but it has been the hardest bug so far for me to wrap my head around, probably because it is on a lower level than I am used to.  One of the thing’s I have been trying to do this semester is to get a working patch up as fast as I can, regardless of how hacky it is.  I’ve been doing this for 2 reasons: 1. It allows me to attempt a lot of bugs and I don’t get hung up on any 1 issue, giving me a chance to think about problems I’m encountering. 2. I get feedback on what I am doing wrong earlier rather than later, allowing me to reduce the possibility of building on an early mistake.  It seems to be working out well so far and the only thing I am concerned with is that I may be putting patches up for review too early, in which case I might be annoying some of the reviewers with an insignificant patch.

I hope to have a patch up for the new bug I have taken on up for review by tonight as wel as hopefully hear from Chris at some point this week. Until then I am going to keep doing what I’m doing and hope for the best.

About these ads
Leave a Comment

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

%d bloggers like this: