Bug 29910 – Load state can get out of sync in a client callback, leading to a crash.

• Component: Page Loading
• Status: NEW

[Yael]

In my specific use case, an attempt is made to load a non-existing page from the local file system.
When the provisional load is cleared, the provisional loader is cleared before the progress callback, but the state is cleared after the progress callback.
A client could e.g. try to set a user stylesheet from inside the progress callback, which will result in a crash - the state is still FrameStateProvisional, but the provisional loader is NULL.

Clearing both states before calling the progress callback eliminates the crash.

I would love to add a new test case for this, but since this is a complex use case, I am not sure how.

[Yael]

Created attachment 40339 [details]
Patch

[Brady Eidson]

Comment on attachment 40339 [details]
Patch

r- because:
1 - I'm not exactly clear what the motivation for the change is
2 - The change is only partial (this pattern appears more than once)
3 - We *NEED* to start enforcing layouttests for loader changes like these.


Elaborations:
1 - Before we jump the gun on this change, I'd like some more details of what the problem actually is.  Your description of the bug sounds theoretical - could you post a crash log and steps to reproduce?
2 - This pattern of setting FrameStateComplete after the progressCompleted() callback occurs more than once, and appears to have been a deliberate decision.  While I have no direct evidence it's the wrong change, my Spidey Sense is tingling.
3 - If we clear up #1 and #2, I'm sure I can help find a way to layouttest this.

[Yael]

Created attachment 40376 [details]
Callstack leading to the crash

[Yael]

Thank you for your review. 

> 3 - We *NEED* to start enforcing layouttests for loader changes like these.

I agree with you that some additional testing should be added. If you could help me understand how to create a test for this complex use case, and I would be happy to do that.
 

> Elaborations:
> 1 - Before we jump the gun on this change, I'd like some more details of what
> the problem actually is.  Your description of the bug sounds theoretical -
> could you post a crash log and steps to reproduce?

This crash happened in S60 environment and I could not attach crash log, but I did attach a callstack leading to the crash. Some explanation of the use case might help:

This crash was observed when I opened the Browser and the first url I tried to load was a non-existing html file from the file system. Inside the progress callback of the first load, the browser sets a new user stylesheet, which leads to a new load request.

The actual crash happens when we select an active document loader to load the user stylesheet.

DocumentLoader* FrameLoader::activeDocumentLoader() const
{
    if (m_state == FrameStateProvisional) 
        return m_provisionalDocumentLoader.get();
    return m_documentLoader.get();
}

The state is still FrameStateProvisional, but m_provisionalDocumentLoader was already set to NULL. This returns a NULL DocumentLoader, leading to a crash.


> 2 - This pattern of setting FrameStateComplete after the progressCompleted()
> callback occurs more than once, and appears to have been a deliberate decision.
>  While I have no direct evidence it's the wrong change, my Spidey Sense is
> tingling.

Could you refer me to where is this pattern used? In FrameLoader.cpp, ProgressComplete() is called from 2 places. The second place is  checkLoadCompleteForThisFrame(), and it is called after markLoadComplete() is called.

> 3 - If we clear up #1 and #2, I'm sure I can help find a way to layouttest
> this.

I appreciate your help here.

[Yael]

Created attachment 40724 [details]
Updated the patch after latest changes to frameLoader.cpp

The crash happens when the client is setting a user stylesheet during a progress comeplete call.
I really don't think there is a way to simulate this sequence with DumpRenderTree.
Given that the fix is so trivial, I think it should be ok without a regression test.

[Eric Seidel (no email)]

Comment on attachment 40724 [details]
Updated the patch after latest changes to frameLoader.cpp

Even if this change is simple, we need to make sure we never have this bug again.

Best would be to have a layout test.  If you really believe a layout test is impossible, you need to explain so in the ChangeLog.  Explain why it's impossible, and what would reproduce this if it were possible.  A WebCore/manual-test is an option, but less desirable.

Additionally, you could/should  add a comment next to the setState(FrameStateComplete), to explain why it needs to be set before progressCompleted is called.

the loader is subtle.  We need to make it less subtle. :)  Your help in doing so is most appreciated!

[Yael]

Created attachment 40733 [details]
Patch

Thank you for your review, Eric.
I added comments to both FrameLoader.cpp and Changelog, as you requested.

[Adam Barth]

Comment on attachment 40733 [details]
Patch

r- for lack of a test.  That progress callback generates a bunch of FrameLoaderClient callbacks.  I'm sure those generate some number of WebKit client callbacks that DumpRenderTree could listen to and respond with a load, triggering the crash.

Don't be shy about extending DumpRenderTree to call the WebKit API in crazy ways.  Ideally we'd have some generalized mechanism for this, but we can start with a one-off test harness if you don't see a way to generalize it.