WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
New Patch
(5.66 KB, patch)
2010-05-11 21:44 PDT
,
Beth Dakin
darin
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Beth Dakin
Comment 1
2010-05-10 15:29:24 PDT
Created
attachment 55608
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug