[webkit-reviews] review denied: [Bug 30457] Allow image requests started from unload handlers to outlive the page : [Attachment 44561] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 30457: Allow image requests started from unload handlers to outlive the
page
https://bugs.webkit.org/show_bug.cgi?id=30457

Attachment 44561: patch
https://bugs.webkit.org/attachment.cgi?id=44561&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Thanks for taking this on. This is looking good!

I have one or two significant design comments and lots of minor naming and
structure comments.

> +    , m_beforeUnloadEventBeingDispatched(false)

Best practice for boolean data and function members is to name them with an
adjective phrase like this "frame loader <xxx>".

So following that, the name would be m_isDispatchingBeforeUnloadEvent.

> +    m_beforeUnloadEventBeingDispatched = true;
>      bool canContinue = shouldContinue && (!isLoadingMainFrame() ||
m_frame->shouldClose());
> +    m_beforeUnloadEventBeingDispatched = false;

Can this ever re-enter? Maybe we should assert
!m_beforeUnloadEventBeingDispatched to catch it if it ever is in a debug build.


> +	   void setOutlivePage(bool outlivePage) { m_outlivePage = outlivePage;
}
> +	   bool outlivePage() const { return m_outlivePage; }

Best practice for getter functions is to make sure their names are not
something that sound like a verb phrase. So these should be:

    setShouldOutlivePage
    shouldOutlivePage
    m_shouldOutlivePage
    m_shouldSkipSecurityCheck

If the thing is named like a verb, then the getter function sounds like a
function that will take action.

> +    , m_createdDuringUnload(isDispatchingUnloadEvent)

Should be m_wasCreatedDuringUnload and wasCreatedDuringUnload for reasons
similar to those mentioned above.

> +    , m_frameForOutlivingRequests(0)

No need for this line of code. RefPtr starts out 0 without an explicit
initializer.

> -    CachedImage(const String& url);
> +    CachedImage(const String& url, bool isDispatchingUnloadEvent);

I don't think this design is quite right. You might already have a CachedImage
because some other page tried to load the same image. The CachedImage itself is
shared among multiple requesters of the image. So it doesn't make logical sense
to have that shared CachedImage object have a boolean that depends on the
timing of the first requestImage call.

If we do need to keep the boolean, I think "wasFirstRequestedDuringUnload" is
more precise than "createdDuringUnload" and draws attention to this curious
rule in a way I think could be helpful in understanding behavior in the future.


I think the right way to do this is probably to look at the outstanding
requests and answer a question about whether at least one or whether all are
last-minute requests rather than storing a boolean in the CachedImage.

> -PassRefPtr<SubresourceLoader> SubresourceLoader::create(Frame* frame,
SubresourceLoaderClient* client, const ResourceRequest& request, bool
skipCanLoadCheck, bool sendResourceLoadCallbacks, bool shouldContentSniff)
> +PassRefPtr<SubresourceLoader> SubresourceLoader::create(Frame* frame,
SubresourceLoaderClient* client, const ResourceRequest& request, bool
skipSecurityCheck, bool outlivePage, bool sendResourceLoadCallbacks, bool
shouldContentSniff)

Name should be "shouldOutlivePage".

> +    void incrementOutlivePageRequestCount();
> +    void decrementOutlivePageRequestCount();
> +    int outlivePageRequestCount() const { return m_outlivePageRequestCount;
}
> +    Frame* frameForOutlivingRequests() { return
m_frameForOutlivingRequests.get(); }

> +    int m_outlivePageRequestCount;
> +    RefPtr<Frame> m_frameForOutlivingRequests;

These names are awkward. We should try to do better. A good technique is to
write out an English sentence explaining what a function does or data is, and
then extract words from the sentence and use them in that order. You can't make
the words "increment outlive page request count" into a sentence, because
"outlive page request" doesn't work well. And I think we could find a name that
doesn't require that wording.

Maybe we can come up with a term for these requests that is clearer than
"outliving requests" and use that term in a lot more of this code. What's a
term that in plain English would sound like this concept? Maybe something crazy
like "last gasp requests"? That's not so good, but I think it's better than
"outliving requests". And I think it's critical that we call them the same
thing in the various functions. Here we call them "outliving requests" and
"outlive page requests" side by side -- we should choose one term or the other.


I'm also concerned that the increment/decrement API here makes it easy to get
off by one and leak. The code that manages the count needs to be extra clear.

> -	   RefPtr<SubresourceLoader> loader =
SubresourceLoader::create(docLoader->doc()->frame(),
> -	       this, resourceRequest, request->shouldSkipCanLoadCheck(),
request->sendResourceLoadCallbacks());
> +	   bool outlivePage = request->outlivePage();
> +	   Frame* frame = outlivePage ? docLoader->frameForOutlivingRequests()
: docLoader->doc()->frame();
> +	   RefPtr<SubresourceLoader> loader = SubresourceLoader::create(frame,
> +	       this, resourceRequest, request->skipSecurityCheck(),
outlivePage, request->sendResourceLoadCallbacks());

I think this would be slightly better if factored differently. The function in
DocLoader would take a Request* argument and return the frame to use for the
load. The policy that it's the "outliving requests" that use a different frame
would be in DocLoader.

> +	       if (outlivePage)
> +		   docLoader->decrementOutlivePageRequestCount();

We have a minor design problem here. This Loader function is one of five
different places that call the decrementOutlivePageRequestCount function. It's
quite difficult to read the Loader code and spot whether there are any
mismatches between increment and decrement. We should look to see if we can
change the structure so that it's tighter and a little more clear that the
increments and decrements will always be balanced.

Maybe the best way to do it would be to pass the Request* to
increment/decrementRequestCount instead of having a separate
increment/decrement for requests that will outlive the page.

> +	       newImage = new CachedImage(sourceURI(attr),
document->docLoader()->frame()->loader()->isDispatchingUnloadEvent());

Can any of the items in this expression be zero? For example, can the frame be
zero?

> -	   bool skipCanLoadCheck = false;
> -	   loadRequest(request, skipCanLoadCheck);
> +	   bool skipSecurityCheck = false;
> +	   loadRequest(request, skipSecurityCheck);

Our best practice for this is now to use an enum rather than a boolean. This
would avoid the curious local variable that is declared simply to give a new
name to "false". Since you are changing all these call sites it might be nice
to move from a boolean to an enum. In fact, the rename from skipCanLoadCheck to
skipSecurityCheck really ought to happen in a separate patch, first. That would
help this work get done more quickly, I think. Smaller patches are better.

> +    bool m_beforeUnloadEventBeingDispatched;
>      bool m_unloadEventBeingDispatched;

Your new boolean should be named m_isDipatchingBeforeUnloadEvent. And at some
point the old boolean should be renamed m_isDispatchingUnloadEvent.

Given all the different code paths, I think we need more test cases. For
example, I could probably remove some of the calls to
decrementOutlivePageRequestCount without making the test fail. We should strive
to have tests that would fail if we do the code wrong.

review- because I'd like to see the CachedImage boolean reconsidered and like
to see quite a few more tests if possible

It would also be good to consider my other comments and suggestions, especially
the one about doing the rename in a separate earlier patch, and the
increment/decrement tightening and simplification.