Bug 86413 - MainResourceLoader::load() always returns true
: MainResourceLoader::load() always returns true
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-05-14 16:46 PST by
Modified: 2012-05-15 16:31 PST (History)


Attachments
patch (3.50 KB, patch)
2012-05-14 16:51 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
make load() non-virtual (3.49 KB, patch)
2012-05-14 17:05 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (3.67 KB, patch)
2012-05-15 10:59 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (3.89 KB, patch)
2012-05-15 13:37 PST, Nate Chapin
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-05-14 16:46:14 PST
...so we should be able to make it return void and remove the codepath handling the false case.
------- Comment #1 From 2012-05-14 16:51:12 PST -------
Created an attachment (id=141812) [details]
patch
------- Comment #2 From 2012-05-14 16:59:10 PST -------
(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?
------- Comment #3 From 2012-05-14 17:02:10 PST -------
(In reply to comment #2)
> (From update of attachment 141812 [details] [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.
------- Comment #4 From 2012-05-14 17:05:40 PST -------
Created an attachment (id=141816) [details]
make load() viritual
------- Comment #5 From 2012-05-15 00:14:43 PST -------
(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
> -    // 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.
------- Comment #6 From 2012-05-15 01:19:48 PST -------
(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.
------- Comment #7 From 2012-05-15 10:07:51 PST -------
(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.
------- Comment #8 From 2012-05-15 10:23:53 PST -------
(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.
------- Comment #9 From 2012-05-15 10:49:40 PST -------
(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 141816 [details] [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.
------- Comment #10 From 2012-05-15 10:59:05 PST -------
Created an attachment (id=142006) [details]
Patch for landing
------- Comment #11 From 2012-05-15 13:03:04 PST -------
(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.
------- Comment #12 From 2012-05-15 13:07:53 PST -------
(In reply to comment #11)
> (From update of attachment 142006 [details] [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.
------- Comment #13 From 2012-05-15 13:16:04 PST -------
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.
------- Comment #14 From 2012-05-15 13:16:17 PST -------
Re: the old comment being vacuous, I agree.
------- Comment #15 From 2012-05-15 13:20:06 PST -------
(From update of attachment 142006 [details])
Fair enough.
------- Comment #16 From 2012-05-15 13:37:16 PST -------
Created an attachment (id=142041) [details]
Patch for landing
------- Comment #17 From 2012-05-15 16:30:59 PST -------
(From update of attachment 142041 [details])
Clearing flags on attachment: 142041

Committed r117181: <http://trac.webkit.org/changeset/117181>
------- Comment #18 From 2012-05-15 16:31:04 PST -------
All reviewed patches have been landed.  Closing bug.