Bug 146786 – DocumentLoader::detachFromFrame() is being called with no current Frame set

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

DocumentLoader::detachFromFrame() is being called with no current Frame set

We're seeing the crashes in <rdar://problem/21293082>

Sample backtrace:
>  1 com.apple.WebCore              0x7fff88ab2985 WebCore::DocumentLoader::detachFromFrame() + 0xa5
   2 com.apple.WebCore              0x7fff88bc77ae WebCore::FrameLoader::clearProvisionalLoad() + 0x1e
   3 com.apple.WebCore              0x7fff88a810e9 WebCore::FrameLoader::checkLoadCompleteForThisFrame() + 0x499
   4 com.apple.WebCore              0x7fff88a80b86 WebCore::FrameLoader::checkLoadComplete() + 0x136
   5 com.apple.WebCore              0x7fff88bc6657 WebCore::FrameLoader::receivedMainResourceError(WebCore::ResourceError const&) + 0x247
   6 com.apple.WebCore              0x7fff88ae73b9 WebCore::CachedResource::checkNotify() + 0x99
   7 com.apple.WebCore              0x7fff88bc6137 WebCore::SubresourceLoader::didFail(WebCore::ResourceError const&) + 0x177
   8 com.apple.WebCore              0x7fff88aca3a5 WebCore::DocumentLoader::continueAfterContentPolicy(WebCore::PolicyAction) + 0x2a5
   9 com.apple.WebCore              0x7fff88aca0ec WebCore::PolicyCallback::call(WebCore::PolicyAction) + 0x1c
  10 com.apple.WebCore              0x7fff88aca0a3 WebCore::PolicyChecker::continueAfterContentPolicy(WebCore::PolicyAction) + 0x183
  11 com.apple.WebKit               0x7fff8b60657a WebKit::WebFrame::didReceivePolicyDecision(unsigned long long, WebCore::PolicyAction, unsigned long long, unsigned long long) + 0xc0
  12 com.apple.WebKit               0x7fff8b6078b3 WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse(WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, std::__1::function<void (WebCore::PolicyAction)>) + 0x2c3
  13 com.apple.WebCore              0x7fff8956a1a6 WebCore::PolicyChecker::checkContentPolicy(WebCore::ResourceResponse const&, std::__1::function<void (WebCore::PolicyAction)>) + 0xe6
  14 com.apple.WebCore              0x7fff88aa8b14 WebCore::DocumentLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) + 0x724
  15 com.apple.WebCore              0x7fff88aa829d WebCore::CachedRawResource::responseReceived(WebCore::ResourceResponse const&) + 0xcd
  16 com.apple.WebCore              0x7fff88aa8019 WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&) + 0x1b9
  17 com.apple.WebKit               0x7fff8b6ab6c5 WebKit::WebResourceLoader::didReceiveResponse(WebCore::ResourceResponse const&, bool) + 0x41

Digging in deeper to the detachFromFrame() frame of the backtrace, the crash is dereferencing m_frame when calling InspectorInstrumentation::loaderDetachedFromFrame

The null frame case means that either:
1 - The DocumentLoader is being detached twice
2 - The DocumentLoader is being detached before ever being attached.

We can't quite figure out which.

The ASSERT(m_frame) at the top of this method is still valid - it is a mistake for this case to ever happen.

[Brady Eidson]

Created attachment 256498 [details]
Patch v1

I'm still going deeper down the rabbit hole trying to theorize how this might happen, but this patch definitely stops this specific crash.

[Brady Eidson]

https://trac.webkit.org/changeset/186642

[Csaba Osztrogonác]

(In reply to comment #2)
> https://trac.webkit.org/changeset/186642

It made 46 tests assert.

[Brady Eidson]

(In reply to comment #3)
> (In reply to comment #2)
> > https://trac.webkit.org/changeset/186642
> 
> It made 46 tests assert.

I've said it before, I'll say it again - EWS is useless in its current form :(

[Brady Eidson]

ACK - DocumentLoaders in the page cache don't detach from their frame  =/

Which means when we bring them out and attach to their new frame, they never undergo a formal "detach from frame" process  =/

[Brady Eidson]

(In reply to comment #5)
> ACK - DocumentLoaders in the page cache don't detach from their frame  =/
> 
> Which means when we bring them out and attach to their new frame, they never
> undergo a formal "detach from frame" process  =/

I see, the clause I removed in the patch covered this - they can only ever come out to the frame they had when going in.

This makes me feel a little better, but not super great.

[Csaba Osztrogonác]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > https://trac.webkit.org/changeset/186642
> > 
> > It made 46 tests assert.
> 
> I've said it before, I'll say it again - EWS is useless in its current form
> :(

I wouldn't say if EWS is useless. But it isn't able to catch assertions 
yet, because we have only release bots and not debug bots too.

You can inspire your company to setup debug EWS bots too. ;)
Anyway you can (or should) watch the buildbots after landing.

[Brady Eidson]

(In reply to comment #7)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > https://trac.webkit.org/changeset/186642
> > > 
> > > It made 46 tests assert.
> > 
> > I've said it before, I'll say it again - EWS is useless in its current form
> > :(
> 
> I wouldn't say if EWS is useless. But it isn't able to catch assertions 
> yet, because we have only release bots and not debug bots too.

This is one aspect of the automation paradox. If the automation doesn't do the whole job, then relying on it at all causes problems.
> 
> You can inspire your company to setup debug EWS bots too. ;)

My comment was actually a flippant reminder to those who know that I've been asking for 100% EWS coverage for a few years now :)

> Anyway you can (or should) watch the buildbots after landing.

The other aspect of the automation paradox - Relying on the automation at all causes us to lose our expertise in doing things manually. :)

That said, we have bot watchers, non of which who pinged me...  yikes!

Anyways, a fix is on its way.

[Brady Eidson]

https://trac.webkit.org/changeset/186677 to restore the frame equality check and resolve the assert.