Bug 237451 – Home link on weather.gov is not working

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Home link on weather.gov is not working in WebKit but works in Chrome & Firefox.

[Chris Dumez]

<rdar://60409277>

[Chris Dumez]

Created attachment 453790 [details]
WIP patch

Causes WPT failures that need investigation.

[Chris Dumez]

Created attachment 453800 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Chris Dumez]

Created attachment 453805 [details]
Patch

[Chris Dumez]

WPT test upstreaming: https://github.com/web-platform-tests/wpt/pull/33053

[Chris Dumez]

Created attachment 453809 [details]
Patch

[Geoffrey Garen]

Comment on attachment 453809 [details]
Patch

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

Looks good!

Comment below about WeakPtr. I guess I am parroting years of Darin explaining to me that basing behaviors on destructors is not a good pattern.

> Source/WebCore/ChangeLog:14
> +        We only only resolve the target history entry once the scheduled task runs asynchronously.

only only

> Source/WebCore/loader/NavigationScheduler.cpp:276
> +        if (!m_historyItem)
> +            return;

I suppose that we could just make the HistoryItem a RefPtr and guarantee that the navigation succeeds.

Is the goal here that the navigation should fail if the history item has been removed from the back-forward list? (Is that required by spec or WPT?)

If that's the goal, I wonder if we should use a RefPtr and an explicit "isInList" boolean, or an explicit scan of the list, rather than a weak pointer. In theory, object lifetime is allowed to do whatever it wants, and a lambda or GC or tree of objects or whatever might change lifetime in unexpected ways, causing a navigation to succeed or fail in surprising ways.

> LayoutTests/TestExpectations:346
> +# This test is timing out in both WebKit in Blink (one of the subtests is timing out in Gecko).

WebKit and Blink

[Chris Dumez]

(In reply to Geoffrey Garen from comment #8)
> Comment on attachment 453809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453809&action=review
> 
> Looks good!
> 
> Comment below about WeakPtr. I guess I am parroting years of Darin
> explaining to me that basing behaviors on destructors is not a good pattern.
> 
> > Source/WebCore/ChangeLog:14
> > +        We only only resolve the target history entry once the scheduled task runs asynchronously.
> 
> only only
> 
> > Source/WebCore/loader/NavigationScheduler.cpp:276
> > +        if (!m_historyItem)
> > +            return;
> 
> I suppose that we could just make the HistoryItem a RefPtr and guarantee
> that the navigation succeeds.
> 
> Is the goal here that the navigation should fail if the history item has
> been removed from the back-forward list? (Is that required by spec or WPT?)

Yes, that's the goal. I made this change to keep one WPT test passing as it was expecting this behavior.

> If that's the goal, I wonder if we should use a RefPtr and an explicit
> "isInList" boolean, or an explicit scan of the list, rather than a weak
> pointer. In theory, object lifetime is allowed to do whatever it wants, and
> a lambda or GC or tree of objects or whatever might change lifetime in
> unexpected ways, causing a navigation to succeed or fail in surprising ways.

I will look into this but scanning the list for example is not a viable solution because the list is only on the UIProcess side and I wouldn't want to introduce new sync IPC to check. I agree that relying on the WeakPtr is a bit fragile though and hopefully I can find something better. Even though the WebProcess doesn't actually have the list, it does maintain a set of all HistoryItems and it removes from that set whenever the UIProcess sends it IPC to tell it the HistoryItem is no longer needed. I can likely set the boolean you suggest then.

> > LayoutTests/TestExpectations:346
> > +# This test is timing out in both WebKit in Blink (one of the subtests is timing out in Gecko).
> 
> WebKit and Blink

[Chris Dumez]

Created attachment 453855 [details]
Patch

[Chris Dumez]

Created attachment 453861 [details]
Patch

[Darin Adler]

(In reply to Geoffrey Garen from comment #8)
> I guess I am parroting years of Darin
> explaining to me that basing behaviors on destructors is not a good pattern.

My comments on this are specifically: behaviors based on the timing of destruction of objects with *shared* ownership (which typically means reference counting or garbage collection). For such shared objects we really want to be as close as possible to "just free memory in the destructor".

If an object has simpler lifetime and is not shared, then work in a destructor *could* be OK, but even in that case it can be tricky to write correct code. Inside destructors an object is typically already partly destroyed, for example if the object is has a polymorphic class, and was an instance of a derived class from this base class.

Since so many of our objects are reference counted, it’s easy to see how my advice could be perceived as applying to all destructors.

[EWS]

Committed r290849 (248082@main): <https://commits.webkit.org/248082@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453861 [details].