Bug 86413 – MainResourceLoader::load() always returns true

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

...so we should be able to make it return void and remove the codepath handling the false case.

[Nate Chapin]

Created attachment 141812 [details]
patch

[Alexey Proskuryakov]

Comment on attachment 141812 [details]
patch

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

> Source/WebCore/loader/MainResourceLoader.h:57
> -        virtual bool load(const ResourceRequest&, const SubstituteData&);
> +        virtual void load(const ResourceRequest&, const SubstituteData&);

Is it also not really virtual?

[Nate Chapin]

(In reply to comment #2)
> (From update of attachment 141812 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141812&action=review
> 
> > Source/WebCore/loader/MainResourceLoader.h:57
> > -        virtual bool load(const ResourceRequest&, const SubstituteData&);
> > +        virtual void load(const ResourceRequest&, const SubstituteData&);
> 
> Is it also not really virtual?

So it appears.

[Nate Chapin]

Created attachment 141816 [details]
make load() non-virtual

[Darin Adler]

Comment on attachment 141816 [details]
make load() non-virtual

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

> Source/WebCore/loader/DocumentLoader.cpp:-842
> -    // Protect MainResourceLoader::load() method chain from clearMainResourceLoader() stomping m_mainResourceLoader.
> -    RefPtr<MainResourceLoader> protectedMainResourceLoader(m_mainResourceLoader);

What proves this is no longer needed? It seems to have nothing to do with the return value from load.

[Alexey Proskuryakov]

Comment on attachment 141816 [details]
make load() non-virtual

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

>> Source/WebCore/loader/DocumentLoader.cpp:-842
>> -    RefPtr<MainResourceLoader> protectedMainResourceLoader(m_mainResourceLoader);
> 
> What proves this is no longer needed? It seems to have nothing to do with the return value from load.

I think that this is just following a general pattern in WebKit - a function that needs to protect a variable needs to do that itself. Since load() is now the last line here, RefPtr is clearly out of place.

An investigation of why this RefPtr was added, and whether that's covered by regression tests would be useful indeed.

[Darin Adler]

Comment on attachment 141816 [details]
make load() non-virtual

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

>>> Source/WebCore/loader/DocumentLoader.cpp:-842
>>> -    RefPtr<MainResourceLoader> protectedMainResourceLoader(m_mainResourceLoader);
>> 
>> What proves this is no longer needed? It seems to have nothing to do with the return value from load.
> 
> I think that this is just following a general pattern in WebKit - a function that needs to protect a variable needs to do that itself. Since load() is now the last line here, RefPtr is clearly out of place.
> 
> An investigation of why this RefPtr was added, and whether that's covered by regression tests would be useful indeed.

I see what you mean. The safe thing would be to move the RefPtr into the load function, given that, not remove the RefPtr. Unless we’re certain the load function doesn’t need it.

[Nate Chapin]

(In reply to comment #7)
> (From update of attachment 141816 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141816&action=review
> 
> >>> Source/WebCore/loader/DocumentLoader.cpp:-842
> >>> -    RefPtr<MainResourceLoader> protectedMainResourceLoader(m_mainResourceLoader);
> >> 
> >> What proves this is no longer needed? It seems to have nothing to do with the return value from load.
> > 
> > I think that this is just following a general pattern in WebKit - a function that needs to protect a variable needs to do that itself. Since load() is now the last line here, RefPtr is clearly out of place.
> > 
> > An investigation of why this RefPtr was added, and whether that's covered by regression tests would be useful indeed.
> 
> I see what you mean. The safe thing would be to move the RefPtr into the load function, given that, not remove the RefPtr. Unless we’re certain the load function doesn’t need it.

I looked briefly before posting the patch and didn't see any problems, but I wasn't as thorough as I should've been. I'll look more carefully and report back, or I'll add the protecting RefPtr to load() before landing.

[Nate Chapin]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 141816 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141816&action=review
> > 
> > >>> Source/WebCore/loader/DocumentLoader.cpp:-842
> > >>> -    RefPtr<MainResourceLoader> protectedMainResourceLoader(m_mainResourceLoader);
> > >> 
> > >> What proves this is no longer needed? It seems to have nothing to do with the return value from load.
> > > 
> > > I think that this is just following a general pattern in WebKit - a function that needs to protect a variable needs to do that itself. Since load() is now the last line here, RefPtr is clearly out of place.
> > > 
> > > An investigation of why this RefPtr was added, and whether that's covered by regression tests would be useful indeed.
> > 
> > I see what you mean. The safe thing would be to move the RefPtr into the load function, given that, not remove the RefPtr. Unless we’re certain the load function doesn’t need it.
> 
> I looked briefly before posting the patch and didn't see any problems, but I wasn't as thorough as I should've been. I'll look more carefully and report back, or I'll add the protecting RefPtr to load() before landing.

The more I look, the less convinced I am that removing the RefPtr was safe. Adding back in and landing.

[Nate Chapin]

Created attachment 142006 [details]
Patch for landing

[Darin Adler]

Comment on attachment 142006 [details]
Patch for landing

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

> Source/WebCore/loader/MainResourceLoader.cpp:657
> +    RefPtr<MainResourceLoader> protect(this);

Might be worthwhile to add a comment here explaining why we think this is needed. There was a comment in startLoadingMainResource, even though it wasn’t particularly clear.

[Nate Chapin]

(In reply to comment #11)
> (From update of attachment 142006 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142006&action=review
> 
> > Source/WebCore/loader/MainResourceLoader.cpp:657
> > +    RefPtr<MainResourceLoader> protect(this);
> 
> Might be worthwhile to add a comment here explaining why we think this is needed. There was a comment in startLoadingMainResource, even though it wasn’t particularly clear.

ap and I briefly debated this in #webkit. I felt the previous comment was pretty vacuous, as it paraphrases to "Prevent us from being deleted by the only place that clears our main RefPtr".

It might be a little clearer, regardless of the presence of a comment, if the RefPtr were in loadNow() instead of load(), but I'm not sure.

[Darin Adler]

Here’s my comment on the comment: Maybe there’s nothing to say in the comment because we know nothing. If we know anything about why this is helpful, saying what little we do know in the comment can help people later.

[Darin Adler]

Re: the old comment being vacuous, I agree.

[Nate Chapin]

Comment on attachment 142006 [details]
Patch for landing

Fair enough.

[Nate Chapin]

Created attachment 142041 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142041 [details]
Patch for landing

Clearing flags on attachment: 142041

Committed r117181: <http://trac.webkit.org/changeset/117181>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.