Bug 38871 – REGRESSION: Crash clicking background NPR tab after few minutes of sitting idle

• Component: Media
• Status: RESOLVED FIXED

[Beth Dakin]

Crash in Page::setCanStartMedia() with the following steps:

1. Open WebKit, and load NPR in the first tab.
2. Open a bunch of other tabs…approximately 15 or so.
3. Let the browser sit for 15-20 minutes.
4. Click back to the NPR tab.
5. Crash.

Patch coming soon.

<rdar://problem/7941504>

[Beth Dakin]

Created attachment 55608 [details]
Patch

[Beth Dakin]

I should mention that I am trying to create a layout test for this.

[Beth Dakin]

I don't know that it is possible to make a test for this. Here is what a test would need to do to reproduce the problem:

1. Create an audio element. This we can obviously do in a layout test.
2. Destroy the audio element. This is only possible by forcing a garbage collection, which we can also do in a layout test.
3. Force Page::setCanStartMedia() to run. This is the part that I am not sure we can simulate in a layout test. This function is only called through WebKit when AppKit-y things are happening.

[Eric Carlson]

Comment on attachment 55608 [details]
Patch

> @@ -526,7 +547,7 @@ void HTMLMediaElement::loadInternal()
>      // If we can't start a load right away, start it later.
>      Page* page = document()->page();
>      if (page && !page->canStartMedia()) {
> -        if (m_isWaitingUntilMediaCanStart)
> +        if (m_isWaitingUntilMediaCanStart || !attached())
>              return;

Are you certain this will work with audio elements created with "new Audio()" (these elements are not in the DOM are playable)?

r=me if you have verified that this continues to work.

[Beth Dakin]

Thanks Eric! I know that the NPR site in question uses "new Audio()," and I also know that they layout tests pass, and at least a couple of them also use "new Audio()," so I feel relatively confident that this still works, but I will do a but more testing for good measure.

[Beth Dakin]

Oh no, I think you're right Eric! I had to set breakpoints to really test this, but it looks like, with my current patch, there is no crash because the audio element from NPR (created with "new Audio()" is never added to the m_mediaCanStartListeners HashMap. Obviously we can take out the !attached() check in HTMLMediaElement::loadInternal(), but then the bug still exists that we never REMOVE the element from the HashMap since we moved that code the detach, which will never be called. 

Looks like attach() and detach() are not the right place for this code after all.

[Beth Dakin]

Comment on attachment 55608 [details]
Patch

r-minusing due to recent findings.

[Darin Adler]

Beth landed a first refactoring step in <http://trac.webkit.org/changeset/59186>.

[Darin Adler]

I landed a second refactoring step: <http://trac.webkit.org/changeset/59189>.

[Beth Dakin]

Patch coming soon!

[WebKit Review Bot]

http://trac.webkit.org/changeset/59189 might have broken Qt Linux Release minimal and Qt Linux ARMv5 Release

[Beth Dakin]

Created attachment 55804 [details]
New Patch

Here's a new patch that follows along with the plan that Darin and I concocted earlier to move the listeners HashSet to Document from Page. All of the layout tests pass, and I was able to catch the former-NPR-crash in the debugger, and I was able to see us properly adding and removing the listener from the HashSet and then not crashing, yay! And basic media browsing seems to work ;-)

[Beth Dakin]

Thanks Darin! Fixed with r59239.