RESOLVED FIXED 38871
REGRESSION: Crash clicking background NPR tab after few minutes of sitting idle
https://bugs.webkit.org/show_bug.cgi?id=38871
Summary REGRESSION: Crash clicking background NPR tab after few minutes of sitting idle
Beth Dakin
Reported 2010-05-10 15:18:33 PDT
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>
Attachments
Patch (5.00 KB, patch)
2010-05-10 15:29 PDT, Beth Dakin
bdakin: review-
New Patch (5.66 KB, patch)
2010-05-11 21:44 PDT, Beth Dakin
darin: review+
Beth Dakin
Comment 1 2010-05-10 15:29:24 PDT
Beth Dakin
Comment 2 2010-05-10 15:37:23 PDT
I should mention that I am trying to create a layout test for this.
Beth Dakin
Comment 3 2010-05-10 16:44:35 PDT
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 4 2010-05-10 21:34:10 PDT
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
Comment 5 2010-05-11 09:44:47 PDT
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
Comment 6 2010-05-11 12:32:37 PDT
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 7 2010-05-11 12:43:22 PDT
Comment on attachment 55608 [details] Patch r-minusing due to recent findings.
Darin Adler
Comment 8 2010-05-11 17:13:07 PDT
Beth landed a first refactoring step in <http://trac.webkit.org/changeset/59186>.
Darin Adler
Comment 9 2010-05-11 17:38:42 PDT
I landed a second refactoring step: <http://trac.webkit.org/changeset/59189>.
Beth Dakin
Comment 10 2010-05-11 17:48:37 PDT
Patch coming soon!
WebKit Review Bot
Comment 11 2010-05-11 18:04:59 PDT
http://trac.webkit.org/changeset/59189 might have broken Qt Linux Release minimal and Qt Linux ARMv5 Release
Beth Dakin
Comment 12 2010-05-11 21:44:02 PDT
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
Comment 13 2010-05-12 09:53:09 PDT
Thanks Darin! Fixed with r59239.
Note You need to log in before you can comment on or make changes to this bug.