Bug 59185 – NULL checking for set request's encoding

• Component: Page Loading
• Status: RESOLVED INVALID

[Grzegorz Czajkowski]

During loading some resources, especially when loading was failed segfault occurred  because activeDocumentLoader() has been already destroyed.
See callstack in an attachment.

It happens when encoding for the request is set and activeDocumentLoader() checks frame's encoding. 

You can check this on www.latimes.com when WebKit gets resource: https://latimes.signon.trb.com/registration/xd/xd_sender.htm

[Grzegorz Czajkowski]

Created attachment 90685 [details]
Callstack

[Grzegorz Czajkowski]

Created attachment 90687 [details]
patch

[Alexey Proskuryakov]

This would need a regression test to be landed.

However, I doubt that this is the right fix. This problem is not known to occur in other ports, so you should look into what destroys activeDocumentLoader too early, and fix that.

[Grzegorz Czajkowski]

(In reply to comment #3)
> This would need a regression test to be landed.
> 
> However, I doubt that this is the right fix. This problem is not known to occur in other ports, so you should look into what destroys activeDocumentLoader too early, and fix that.

Ok, the reason of this issue was found.
Anyway methods:
 - FrameLoader::isLoading()
 - FrameLoader::checkLoadCompleteForThisFrame()
checks returned value of activeDocumentLoader() but some of them in FrameLoader not. Do you think that it's safe to have an access to the object if it may return NULL?

[Alexey Proskuryakov]

Adding checks when we know that the returned value can't be null would be very confusing to readers. By having a null check, we're saying that the situation is expected, and we need to handle it adequately. The document encoding is an important part of fallback, so skipping it is not adequate.

We would need to find another way to retrieve the document encoding if activeDocumentLoader could be null here.

[Grzegorz Czajkowski]

Ok, thanks for your explanation. I think we can close this bug as invalid.