| Summary: | NSAttributedString conversion in a loop returns nil and WKUnknownError every other time | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||
| Component: | WebKit API | Assignee: | Timothy Hatcher <timothy> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | bdakin, commit-queue, darin, thorton, webkit-bug-importer, wenson_hsieh | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Timothy Hatcher
2019-05-02 11:30:48 PDT
Created attachment 368812 [details]
Patch
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? Created attachment 368830 [details]
Patch
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. Comment on attachment 368830 [details] Patch Clearing flags on attachment: 368830 Committed r244893: <https://trac.webkit.org/changeset/244893> All reviewed patches have been landed. Closing bug. |