Bug 42610 – NotificationCenter::disconnectFrame will crash if called twice

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[John Gregg]

NotificationCenter::disconnectFrame will crash if called twice

[John Gregg]

Created attachment 62024 [details]
Patch

[WebKit Review Bot]

Attachment 62024 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[John Gregg]

Created attachment 62026 [details]
Patch

[Andreas Kling]

Related: 42534

[Alexey Proskuryakov]

> Adds a null check; this is a prospective fix for a crash that is difficult to repro.

Why is it difficult to reproduce? This looks like a case where having a regression test is particularly important.

> NotificationCenter::disconnectFrame will crash if called twice

How can it be called twice?

[John Gregg]

(In reply to comment #5)
> > Adds a null check; this is a prospective fix for a crash that is difficult to repro.
> 
> Why is it difficult to reproduce? This looks like a case where having a regression test is particularly important.
> 

The crash is being reported by Chrome users, with a stack trace that suggests this code path, and the timing suggests http://trac.webkit.org/changeset/62939 may be the culprit.   When I say difficult to repro, actually I have not been able to reproduce it at all, but because of the crash reports, we know it is happening.

> > NotificationCenter::disconnectFrame will crash if called twice
> 
> How can it be called twice?

Again, not sure that's what's happening.  But if it did happen, it will definitely crash.  But as Andreas pointed out, bug 42534 has more information.

[Alexey Proskuryakov]

There are multiple disconnectFrame() calls in various objects, all called from DOMWindow::clear(), and those don't seem to cause crashes.

One way to attack this is to answer the question of what is different about NotificationCenter.

[John Gregg]

I think Yael understands the cause of the crash in bug 42534, so I will withdraw this patch for now.

[Yael]

Not being able to reproduce this is slowing me down, and if someone can find a way to reproduce, I will be happy to know about it. 
I am working on this, and sorry for the inconvenience.

[Yael]

(In reply to comment #7)
> One way to attack this is to answer the question of what is different about NotificationCenter.
This is a very good question :-)
Since notifications are expected to outlive the page that created them, I made Qt's NotificationPresenter a singleton. It is deleted when the last page is deleted.
John, is it a singleton in Chromium too?

[Yael]

*** This bug has been marked as a duplicate of bug 42534 ***