Bug 33188 – ER: Need a way to know when FrameLoader is loading a URL based on user action

• Component: Page Loading
• Status: NEW

[David Kilzer (:ddkilzer)]

Created attachment 45844 [details]
Brain-dead implementation using FrameLoader::isProcessingUserGesture()

* SUMMARY
In order to display the "Download Failed" alert in MobileSafari, we need to know if a user tapped on a link within an <iframe> or <frame> element versus the URL being loaded as part of the HTML source or being loaded automatically using JavaScript (document.write or setting the location in the frame's document).

This essentially translates to storing the userGesture flag in FrameLoader::urlSelected() and resetting it elsewhere.  I've attached a patch that works (on the surface), but it sets m_userNavigation to false twice in some code paths, and sets it three times (twice to false and once to the value of 'userGesture') in the code path we're interested in.  It also assumes that modifying FrameLoader::isProcessingUserGesture() is sufficient to check the value of m_userNavigation.

Basically, I need to figure out the best place to save and reset m_userNavigation, and whether it's acceptable to check it in FrameLoader::isProcessingUserGesture().

[David Kilzer (:ddkilzer)]

<rdar://problem/7509146>

[David Kilzer (:ddkilzer)]

Created attachment 45848 [details]
Patch v1

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45848 [details] without any errors.

[Sam Weinig]

Do you plan on exposing this ability as API/SPI?  That would seem to be necessary to, at the very least, test this patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Do you plan on exposing this ability as API/SPI?  That would seem to be
> necessary to, at the very least, test this patch.

Yes, I was going to add -(BOOL)_isProcessingUserGesture:(WebFrame *)frame in WebViewPrivate.h.  Should that be included in this patch?  Is there a best practice for testing ObjC API in DRT?

Also, I wasn't sure if "clicking" in DRT emulated actual user clicking or whether it would show up as a non-user-gesture.

[Adam Barth]

Comment on attachment 45848 [details]
Patch v1

We aren't accepting FrameLoader patches without tests.

That said, this code is wrong, mostly because all our user gesture code is wrong.  Why is frame->script()->processingUserGesture() insufficient?  I think the correct fix is to teach that function to return the correct answer.

Basically, the right way to do this is when an event enters WebKit (or maybe WebCore), we know whether it's a real user input event or a fake event.  At that time, we should create a scoped object that stores the user gesture state in a static.  script()->processingUserGesture should then read the static.  The current approach of trying to recover the information from Event is wrong and doesn't work in edge cases.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> (From update of attachment 45848 [details])
> We aren't accepting FrameLoader patches without tests.
> 
> That said, this code is wrong, mostly because all our user gesture code is
> wrong.  Why is frame->script()->processingUserGesture() insufficient?  I think
> the correct fix is to teach that function to return the correct answer.

Currently, the userGesture flag is set only when ScriptController::executeIfJavaScriptURL() is called, and thus only when a javascript: URL is clicked.  I need this to work with plain (non-javascript) links as well, which it doesn't today.

> Basically, the right way to do this is when an event enters WebKit (or maybe
> WebCore), we know whether it's a real user input event or a fake event.  At
> that time, we should create a scoped object that stores the user gesture state
> in a static.  script()->processingUserGesture should then read the static.  The
> current approach of trying to recover the information from Event is wrong and
> doesn't work in edge cases.

At what scope should this object exist?  Inside the ScriptController?  I'm not sure what you mean exactly by a "scoped object".

[David Kilzer (:ddkilzer)]

(In reply to comment #5)
> Yes, I was going to add -(BOOL)_isProcessingUserGesture:(WebFrame *)frame in
> WebViewPrivate.h.  Should that be included in this patch?  Is there a best
> practice for testing ObjC API in DRT?

I guess I can add a JavaScript accessor to get the isProcessingUserGesture() value for the current Document.

[Adam Barth]

(In reply to comment #7)
> At what scope should this object exist?  Inside the ScriptController?  I'm not
> sure what you mean exactly by a "scoped object".

It should exist in the stack frame where we receive the event from the outside world.  By "scoped object," I meant it twiddles the static state on construction and destruction.

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Do you plan on exposing this ability as API/SPI?  That would seem to be
> necessary to, at the very least, test this patch.

I found a nice way to test this in DRT, similar to the way frame load delegate callbacks are tested, after adding -_isProcessingUserGestureForFrame:.

Writing tests to verify existing behavior first.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> I found a nice way to test this in DRT, similar to the way frame load delegate
> callbacks are tested, after adding -_isProcessingUserGestureForFrame:.
> 
> Writing tests to verify existing behavior first.

Ugh.  The first test that emulates a user's mouse click (and thus causes -_isProcessingUserGestureForFrame: to return YES) causes subsequent tests to fail because some internal state isn't cleared.  Not sure where it's happening yet.

[David Kilzer (:ddkilzer)]

After writing a few tests, I think FrameLoader::isProcessingUserGesture() answers a different question than I'm asking.

We call ScriptController::isProcessingUserGesture() via FrameLoader because we want to know if the currently running JavaScript code was explicitly started by user action (and not implicitly as part of an page load, for example).  That's exactly what we want to know when asking the question:  Should we open a new window when calling window.open()?  That's because we ask it during the JavaScript code execution.

However, it's the wrong question to ask when you want to know if the content currently being loaded (or that was previously loaded) in a frame was due to explicit user action (which is the new concept that I originally created this bug for).

So where to go from where?  I'm thinking:

- Write tests for isProcessingUserGesture() when calling window.open().
- Introduce new concept (FrameLoader::wasLoadedByUserGesture() or similar) with tests.

[Adam Barth]

I see.  Yes, that's a different concept.  I think we should add a wasLoadedByUserGesture.  It's important to write good tests for this feature because it's going to be subtle and easy to break.

In general, I wish there was a good way to attach context information to loads that persisted through the various machinations of the loading machine.  We don't have a good way to do that at the moment, but this might be a good time to head in that direction.

[Adam Barth]

Is this the same bug as Bug 33381?

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> Is this the same bug as Bug 33381?

It appears to be.  See Bug 33381 Comment #6.

[Steve Block]

Thanks for pointing me to this bug.

(In reply to comment #12)
> - Introduce new concept (FrameLoader::wasLoadedByUserGesture() or similar) with
tests.
Are you sure that using a flag on the FrameLoader is the right thing to do?
Some of the pathways through FrameLoader to load a resource are asynchronous,
so making sure that the flag is correct when the asynchronous callbacks in
FrameLoader are executed would be tricky. That's why I proposed adding the flag
to the ResourceRequest, so it can carry the flag with it as it moves through
FrameLoader.

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> (In reply to comment #12)
> > - Introduce new concept (FrameLoader::wasLoadedByUserGesture() or similar) with
> tests.
> Are you sure that using a flag on the FrameLoader is the right thing to do?
> Some of the pathways through FrameLoader to load a resource are asynchronous,
> so making sure that the flag is correct when the asynchronous callbacks in
> FrameLoader are executed would be tricky. That's why I proposed adding the flag
> to the ResourceRequest, so it can carry the flag with it as it moves through
> FrameLoader.

I am not sure.  Adding a flag was my initial thought, but I didn't have time to investigate all of the various entry points into FrameLoader to make sure the flag was always correct when checked.  I've since got pulled away to work on other issues, so I probably won't get back to this very soon.

[David Kilzer (:ddkilzer)]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #12)
> > > - Introduce new concept (FrameLoader::wasLoadedByUserGesture() or similar) with
> > tests.
> > Are you sure that using a flag on the FrameLoader is the right thing to do?
> > Some of the pathways through FrameLoader to load a resource are asynchronous,
> > so making sure that the flag is correct when the asynchronous callbacks in
> > FrameLoader are executed would be tricky. That's why I proposed adding the flag
> > to the ResourceRequest, so it can carry the flag with it as it moves through
> > FrameLoader.
> 
> I am not sure.  Adding a flag was my initial thought, but I didn't have time to
> investigate all of the various entry points into FrameLoader to make sure the
> flag was always correct when checked.  I've since got pulled away to work on
> other issues, so I probably won't get back to this very soon.

A UserGestureIndicator object was added in r57045 for Bug 37008.  Perhaps a similar concept could be used instead of plumbing a state variable through every method?