Bug 231000 – Add weakThis check in addition to null check added in r282881

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

[Alex Christensen]

Remove log added in r282881

[Alex Christensen]

Created attachment 439685 [details]
Patch

[Alex Christensen]

Created attachment 439686 [details]
Patch

[Chris Dumez]

Comment on attachment 439686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439686&action=review

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:353
>          LOADER_RELEASE_LOG("startNetworkLoad: Going to the network (description=%" PUBLIC_LOG_STRING ")", m_networkLoad->description().utf8().data());

What does m_networkLoad->description() return? Hopefully it doesn't contain anything sensitive like URL?

Also, why can't we move this LOG line *before* calling start()?

[Brady Eidson]

Comment on attachment 439686 [details]
Patch

This patch isn't about verifying the string returns the right thing.

Putting the log line before start() makes it categorically false, as start() may not actually *start*

[Chris Dumez]

(In reply to Brady Eidson from comment #4)
> Comment on attachment 439686 [details]
> Patch
> 
> This patch isn't about verifying the string returns the right thing.

And yet still a good thing to verify and you didn't answer.

> Putting the log line before start() makes it categorically false, as start()
> may not actually *start*

I feel like a safer fix would have been to update the actual code that introduced the regression and have NetworkLoad::start() call didCompleteWithError: asynchronously when it fails due to FTP. There are other clients to NetworkLoad than NetworkResourceLoader and having start() call that may fail synchronously and delete |this| might be an issue for other call sites as well.

[Chris Dumez]

(In reply to Chris Dumez from comment #5)
> (In reply to Brady Eidson from comment #4)
> > Comment on attachment 439686 [details]
> > Patch
> > 
> > This patch isn't about verifying the string returns the right thing.
> 
> And yet still a good thing to verify and you didn't answer.

0x105d74000 - [pageProxyID=21, webPageID=22, frameID=3, resourceID=32, isMainResource=1, destination=3, isSynchronous=0] NetworkResourceLoader::startNetworkLoad: Going to the network (description=LocalDataTask <25187A1B-3324-4E2C-982A-AB468520286C>.<1>)

Now I have my answer.

[EWS]

Committed r283294 (242319@main): <https://commits.webkit.org/242319@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439686 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83699294>

[Brent Fulgham]

*** Bug 230767 has been marked as a duplicate of this bug. ***