Bug 42217 – Webkit crashes while loading gallery.me.com

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

Created attachment 61439 [details]
Output of the Problem Report

Clicking the links in the above thread attempts to open http://gallery.me.com/seththomas#100015/Deck

I am attaching the text of the Webkit Problem Report, I apologize if this isn't required, this is my first problem submittal:

Best Regards,
KEF

[Alexey Proskuryakov]

With a debug build, I got a crash here:

#0	0x01e14c5e in WebCore::DocLoader::decrementRequestCount at DocLoader.cpp:383
#1	0x02460260 in WebCore::Loader::Host::cancelPendingRequests at loader.cpp:594
#2	0x02461389 in WebCore::Loader::Host::cancelRequests at loader.cpp:604
#3	0x02461dab in WebCore::Loader::cancelRequests at loader.cpp:269
#4	0x01f7a483 in WebCore::FrameLoader::stopLoading at FrameLoader.cpp:485
#5	0x01f7a530 in WebCore::FrameLoader::closeURL at FrameLoader.cpp:519
#6	0x01f7a572 in WebCore::FrameLoader::detachFromParent at FrameLoader.cpp:2625
#7	0x0192c558 in -[WebView(WebPrivate) _close] at WebView.mm:1100

> I am attaching the text of the Webkit Problem Report, I apologize if this isn't required

This is a very useful thing to do, see <http://webkit.org/quality/bugwriting.html> and <http://webkit.org/quality/crashlogs.html>. A text attachment would be slightly easier to work with than a PDF one, but it's not a big deal.

[Alexey Proskuryakov]

I believe this was fixed in <http://trac.webkit.org/changeset/63204>. Does this still happen for you with the newest nightly build?

Leon, Pavel: I don't understand why that change didn't have a regression test. ChangeLog said "correcting aspects of the prefetch change that shouldn't have affected functionality" - but avoiding crashes is certainly something that affects functionality!

(In reply to comment #2)
> I believe this was fixed in <http://trac.webkit.org/changeset/63204>. Does this still happen for you with the newest nightly build?
> 
> Leon, Pavel: I don't understand why that change didn't have a regression test. ChangeLog said "correcting aspects of the prefetch change that shouldn't have affected functionality" - but avoiding crashes is certainly something that affects functionality!

Alexey, I have gone back and checked the links with the latest build and the issue is no longer present

Regards,
KEF

[Pavel Feldman]

(In reply to comment #2)
> I believe this was fixed in <http://trac.webkit.org/changeset/63204>. Does this still happen for you with the newest nightly build?
> 
> Leon, Pavel: I don't understand why that change didn't have a regression test. ChangeLog said "correcting aspects of the prefetch change that shouldn't have affected functionality" - but avoiding crashes is certainly something that affects functionality!

@ap: Not sure I understand what exactly you are asking.

(In reply to comment #4)
> (In reply to comment #2)
> > I believe this was fixed in <http://trac.webkit.org/changeset/63204>. Does this still happen for you with the newest nightly build?
> > 
> > Leon, Pavel: I don't understand why that change didn't have a regression test. ChangeLog said "correcting aspects of the prefetch change that shouldn't have affected functionality" - but avoiding crashes is certainly something that affects functionality!
> 
> @ap: Not sure I understand what exactly you are asking.

Nothing - I am only commenting that I went back to the links the generated the crash, and with the latest build I can no longer create the error. 

Regards,
KEF

[Alexey Proskuryakov]

> @ap: Not sure I understand what exactly you are asking.

My question was why r63204 didn't have a regression test - the explanation in ChangeLog didn't make sense to me, as explained above. I cc'ed you since you reviewed that patch.

It would be good to add a regression test.

[Pavel Feldman]

> It would be good to add a regression test.

Hm. Have you seen the patch itself? Previous commit from that bug actually made some tests fail on Chromium Win Debug. I think those were layout tests, although I am not sure anymore. WebKit tree was green for sure at that time.

Anyways, given the nature of the fix and complexity of the potential layout test (how do we address accessing freed memory in layout tests?), I didn't think it made sense. Not sure what "correcting aspects of the prefetch change that shouldn't have affected functionality" was supposed to mean though.

[Alexey Proskuryakov]

> Have you seen the patch itself? 

Yes, the lack of tests looked suspicious to me even before I saw this bug, but I didn't speak up until I saw that the symptom was user observable (yes, it is possible that some early deletes don't cause crashes, at least unless there is another thread doing memory allocation at the moment).

> Previous commit from that bug actually made some tests fail on Chromium Win Debug.

OK, then we already have regression tests for this change, and it's only a ChangeLog entry that was misleading. 

> how do we address accessing freed memory in layout tests

Sometimes, it suffices to add an assertion that catches a bug in debug builds, although that's a pretty weak regression test. Other times, it's actually possible to empirically find a test that crashes.

Being unable to make a regression test is a fair explanation that is up to a reviewer to assess. In this case, I'd have likely agreed that's it was impossible. But this bug would have proven us wrong, since it should be possible to reduce this real life case to a regression test.

[Leon Clarke]

I apologize for the confusing changelog message.

I had a number of reasons for not providing a layout test, but in the changelog I unclearly wrote about one of the less appropriate ones.

What should have been my reason for not including a layout test was that at the time I submitted the CL, I knew this was likely to be a serious problem but didn't have a single report of it actually crashing.  I should have just said that the fix was obvious and I didn't have a report of it failing yet so I couldn't write a layout test.