Bug 147678

Summary: REGRESSION (r185111): Clicking phone numbers doesn't prompt to call sometimes
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit2Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, andersca, ap, beidson, cdumez, commit-queue, ddkilzer, japhet, mitz
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 145280    
Bug Blocks:    
Attachments:
Description Flags
Example
none
Patch and unit test
none
Patch and unit test beidson: review+

Description Daniel Bates 2015-08-04 21:25:14 PDT
Created attachment 258262 [details]
Example

<rdar://problem/21827815>

Following the patch for bug #145280, clicking on phone links (tel URLs) may not work. On iOS, you can use the attached example page to observe this issue by performing the following:

1. Open example.html in MobileSafari.
2. Click the button "Call +1 (408) 555-0123 in nested zero timer callback".
3. Quit and launch MobileSafari again.
4. Click the button "Call +1 (408) 555-0123 in nested zero timer callback".

Then nothing will happen. Compare the behavior after step 4 with the behavior observed in step 2.
Comment 1 Daniel Bates 2015-08-04 21:28:34 PDT
Created attachment 258265 [details]
Patch and unit test
Comment 2 Brady Eidson 2015-08-04 21:37:51 PDT
Comment on attachment 258265 [details]
Patch and unit test

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

> Source/WebCore/loader/HistoryController.cpp:196
> +        if (DocumentLoader* documentLoader = document.loader())
> +            item->setShouldOpenExternalURLsPolicy(documentLoader->shouldOpenExternalURLsPolicyToPropagate());
> +

This when saving document state...

> Source/WebCore/loader/HistoryController.cpp:237
> +    m_frame.loader().documentLoader()->setShouldOpenExternalURLsPolicy(m_currentItem->shouldOpenExternalURLsPolicy());
> +

...combined with this seems like it could be problematic, because if it is possible to have "saved document state" to a history item without a DocumentLoader, therefore the policy was not set, that means it's possible to pull an invalid policy out of the history item later.
Comment 3 Brady Eidson 2015-08-04 21:40:20 PDT
(In reply to comment #2)
> Comment on attachment 258265 [details]
> Patch and unit test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258265&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:196
> > +        if (DocumentLoader* documentLoader = document.loader())
> > +            item->setShouldOpenExternalURLsPolicy(documentLoader->shouldOpenExternalURLsPolicyToPropagate());
> > +
> 
> This when saving document state...
> 
> > Source/WebCore/loader/HistoryController.cpp:237
> > +    m_frame.loader().documentLoader()->setShouldOpenExternalURLsPolicy(m_currentItem->shouldOpenExternalURLsPolicy());
> > +
> 
> ...combined with this seems like it could be problematic, because if it is
> possible to have "saved document state" to a history item without a
> DocumentLoader, therefore the policy was not set, that means it's possible
> to pull an invalid policy out of the history item later.

Looking at Document::loader(), if the Frame returns the Document, then the DocumentLoader will almost certainly *not* be null. The only cases where it might be null are cases where we are already in a grotesquely inconsistent state, so I'm not worried about this any longer.
Comment 4 Brady Eidson 2015-08-04 21:43:52 PDT
Comment on attachment 258265 [details]
Patch and unit test

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

Looks good, with one requested change.

> Source/WebCore/loader/NavigationAction.h:47
> +    NavigationAction(const ResourceRequest&, FrameLoadType, bool isFormSubmission, ShouldOpenExternalURLsPolicy);

A cleanup project of mine very soon is going to be *reducing* the number of NavigationAction constructors (ideally to one), forcing everybody who ever creates one to think about every argument critically.

I'd rather see the three call sites of this new constructor pass in the values you pass in this constructor's implementation, instead of adding this constructor now just to be removed soon.
Comment 5 Daniel Bates 2015-08-04 22:07:44 PDT
Created attachment 258267 [details]
Patch and unit test
Comment 6 Daniel Bates 2015-08-05 09:06:10 PDT
Committed r187962: <http://trac.webkit.org/changeset/187962>