Bug 197523 – NSAttributedString conversion in a loop returns nil and WKUnknownError every other time

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Timothy Hatcher]

This only happens on iOSMac for some reason.

<rdar://problem/50285662>

[Timothy Hatcher]

Created attachment 368812 [details]
Patch

[Darin Adler]

Comment on attachment 368812 [details]
Patch

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

> Source/WebKit/ChangeLog:12
> +        would be confused with the real content loading. Ultimatly this wasn't needed
> +        since a process swap will likely happen, and cached views are closed quickly.
> +        It also avoids extra work and speeds up conversions a bit.

Typo: Ultimatly

The word "likely" here does not inspire confidence in me that this code is correct. How can something that will "likely" happen make something not needed. It seems that it will make it "likely not needed", which is not good enough for correctness.

There must be some other reason it’s not needed.

> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:211
> +    result.string = attributedStringFromRange(rangeOfContents(*m_page->mainFrame().document()->documentElement()), &documentAttributes);

I think the old code would fail cleanly if documentElement() was null, so maybe we want to preserve that property. Or is there a guarantee that documentElement() is non-null?

[Timothy Hatcher]

Created attachment 368830 [details]
Patch

[Timothy Hatcher]

Comment on attachment 368812 [details]
Patch

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

>> Source/WebKit/ChangeLog:12
>> +        It also avoids extra work and speeds up conversions a bit.
> 
> Typo: Ultimatly
> 
> The word "likely" here does not inspire confidence in me that this code is correct. How can something that will "likely" happen make something not needed. It seems that it will make it "likely not needed", which is not good enough for correctness.
> 
> There must be some other reason it’s not needed.

A process swap might not happen if a same origin conversion happens. But that isn't what fixed this issue, a process swap just makes the extra work I was trying to do to clean things up with an about:blank load pointless.

I reworded this in the new patch.

>> Source/WebKit/WebProcess/WebPage/Cocoa/WebPageCocoa.mm:211
>> +    result.string = attributedStringFromRange(rangeOfContents(*m_page->mainFrame().document()->documentElement()), &documentAttributes);
> 
> I think the old code would fail cleanly if documentElement() was null, so maybe we want to preserve that property. Or is there a guarantee that documentElement() is non-null?

Good point. I didn't hit a null case, but it could be. I added a null check on documentElement. I'm still using rangeOfContents() for its simplicity.

[WebKit Commit Bot]

Comment on attachment 368830 [details]
Patch

Clearing flags on attachment: 368830

Committed r244893: <https://trac.webkit.org/changeset/244893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.