Bug 86413 - MainResourceLoader::load() always returns true
: MainResourceLoader::load() always returns true
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Nate Chapin
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-14 16:46 PDT by Nate Chapin
Modified: 2012-05-15 16:31 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2012-05-14 16:46:14 PDT
...so we should be able to make it return void and remove the codepath handling the false case.
Comment 1 Nate Chapin 2012-05-14 16:51:12 PDT
Created attachment 141812 [details]
patch
Comment 2 Alexey Proskuryakov 2012-05-14 16:59:10 PDT
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?
Comment 3 Nate Chapin 2012-05-14 17:02:10 PDT
(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.
Comment 4 Nate Chapin 2012-05-14 17:05:40 PDT
Created attachment 141816 [details]
make load() non-virtual
Comment 5 Darin Adler 2012-05-15 00:14:43 PDT
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.
Comment 6 Alexey Proskuryakov 2012-05-15 01:19:48 PDT
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.
Comment 7 Darin Adler 2012-05-15 10:07:51 PDT
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.
Comment 8 Nate Chapin 2012-05-15 10:23:53 PDT
(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.
Comment 9 Nate Chapin 2012-05-15 10:49:40 PDT
(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.
Comment 10 Nate Chapin 2012-05-15 10:59:05 PDT
Created attachment 142006 [details]
Patch for landing
Comment 11 Darin Adler 2012-05-15 13:03:04 PDT
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.
Comment 12 Nate Chapin 2012-05-15 13:07:53 PDT
(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.
Comment 13 Darin Adler 2012-05-15 13:16:04 PDT
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 Darin Adler 2012-05-15 13:16:17 PDT
Re: the old comment being vacuous, I agree.
Comment 15 Nate Chapin 2012-05-15 13:20:06 PDT
Comment on attachment 142006 [details]
Patch for landing

Fair enough.
Comment 16 Nate Chapin 2012-05-15 13:37:16 PDT
Created attachment 142041 [details]
Patch for landing
Comment 17 WebKit Review Bot 2012-05-15 16:30:59 PDT
Comment on attachment 142041 [details]
Patch for landing

Clearing flags on attachment: 142041

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