| Summary: | Crash in Notification::disconnectFrame() triggered by Frame::lifeSupportTimerFired() | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | ap, commit-queue, darin, fishd, hausmann, johnnyg, laszlo.gombos, yael | ||||||||||||||
| Priority: | P1 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Andreas Kling
2010-07-18 21:03:29 PDT
I should say that the crash dump is from a valgrind run of WebKit/qt/tests/qwebframe/tst_qwebframe Sorry about that. disconnectFrame() is called after the frame was detached from everything else. I need to find a better place to cancel notifications, before the frame is detached. Kling, I applied the patch from https://bugs.webkit.org/show_bug.cgi?id=41065, but the Qt runtime tests were still crashing. After disabling the 3 tests: getSetDynamicProperty(), getSetDynamicProperty_data(), and getSetStaticProperty() I get 2 unrelated XFail, but no crash. Can you please check if you had other code changes in your environment that I should apply to mine? thanks, This is also causing: http://code.google.com/p/chromium/issues/detail?id=49323 *** Bug 42610 has been marked as a duplicate of this bug. *** Created attachment 62184 [details] Patch Call NotificationsCenter::disconnectFrame when a frame is detached from its page. We want to cancel any outstanding premision requests at that point. Since I could not reproduce the crash, I did not add tests at this time, but there is a new test for canceling notifications requests in https://bugs.webkit.org/show_bug.cgi?id=41413 . + if (!presenter())
+ return;
m_notificationPresenter->cancelRequestsForPermission(m_scriptExecutionContext);
Is this null check still needed with other changes?
It would probably be better to use m_notificationPresenter consistently - it's not immediately obvious if presenter() is the same thing.
(In reply to comment #7) > + if (!presenter()) > + return; > m_notificationPresenter->cancelRequestsForPermission(m_scriptExecutionContext); > > Is this null check still needed with other changes? > I think it is safe to add this check, especially since I don't have exact steps to reproduce the error. > It would probably be better to use m_notificationPresenter consistently - it's not immediately obvious if presenter() is the same thing. I agree, I'll update the patch. Created attachment 62188 [details] Patch Change presenter() to m_notificationPresenter, per comment #7. We usually add a comment and an assertion when introducing a null check that probably shouldn't be there. Created attachment 62193 [details] Patch Added comment about the NULL check, per comment #10. I am reluctant to add an ASSERT here, because if m_notificationPresenter will be NULL, Chromium will continue to crash and we will continue to get crash reports. I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit. Created attachment 62195 [details]
Patch
Attached the wrong patch :-)
> I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit.
This sounds surprising - I never heard that any vendor shipped a debug build of WebKit to users.
(In reply to comment #13) > > I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit. > > This sounds surprising - I never heard that any vendor shipped a debug build of WebKit to users. I don't know where these crash logs come from. Maybe someone from Chromium project could tell us. However, if you look at all other functions of NotificationCenter, there is a NULL check in each one of them, so I don't understand why it is a problem to have a NULL check in this new function as well? I don't really like the name DOMWindow::clearNotificationRequests. The method does more than that, since it also sets the NotificationCenter to 0. Perhaps 'disconnectNotifications()' would be more specific. (In reply to comment #14) > (In reply to comment #13) > > > I assume these crash reports are generated from debug builds, not release builds, so the ASSERT will be hit. > > > > This sounds surprising - I never heard that any vendor shipped a debug build of WebKit to users. > > I don't know where these crash logs come from. Maybe someone from Chromium project could tell us. > However, if you look at all other functions of NotificationCenter, there is a NULL check in each one of them, so I don't understand why it is a problem to have a NULL check in this new function as well? The crash reports are from release builds, not debug builds, so I think that the ASSERT would be fine. Created attachment 62209 [details]
Patch
Added NULL check and renamed to disconnectNotifications instead of DOMWindow::clearNotificationRequests.
Comment on attachment 62209 [details] Patch > + // Due to the misterious bug http://code.google.com/p/chromium/issues/detail?id=49323. Spelling error here: "misterious". > +void DOMWindow::disconnectNotifications() This function should be named after when it's called, the event the DOMWindow is learning about, rather than what it does. I think pageDestroyed is a fine name for it. It should not be notifications-specific. The body can be, but the function itself should not. > +#if ENABLE(NOTIFICATIONS) > + if (m_domWindow) > + m_domWindow->disconnectNotifications(); > +#endif Then this could be: if (m_domWindow) m_domWindow->pageDestroyed(); Please make those changes. Created attachment 62212 [details] Patch Fix spelling error (gedit should have built-in spell checker:-)) Rename disconnectNotifications to pageDestroyed, per comment #18. Comment on attachment 62212 [details] Patch Clearing flags on attachment: 62212 Committed r63847: <http://trac.webkit.org/changeset/63847> All reviewed patches have been landed. Closing bug. Revision r63847 cherry-picked into qtwebkit-2.1 with commit 348c731f9f4ab48d4cc2f8aa8f51e00c1f97a850 |