Bug 185532 – Make sure history navigations reuse the existing process when necessary.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Make sure history navigations reuse the existing process when necessary.

<rdar://problem/39746516>

[Brady Eidson]

Created attachment 340140 [details]
Patch

[EWS Watchlist]

Attachment 340140 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/NavigationActionData.cpp:119:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1389:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1391:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1399:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1404:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

/Volumes/Data/EWS/WebKit/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1414:37: error: use of undeclared identifier 'kWKCacheModelDocumentViewer'
    WKContextSetCacheModel(context, kWKCacheModelDocumentViewer);

[Brady Eidson]

Dangit, can't use the C-API for this. Will add Obj-C SPI in new patch

[Brady Eidson]

Dangit, can't use the C-API for this. Will add Obj-C SPI in new patch

[Brady Eidson]

Created attachment 340151 [details]
Patch

[EWS Watchlist]

Attachment 340151 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/NavigationActionData.cpp:119:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1389:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1391:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1394:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1399:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 5 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 340151 [details]
Patch

Clearing flags on attachment: 340151

Committed r231704: <https://trac.webkit.org/changeset/231704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40164172>

[Ali Juma]

Comment on attachment 340151 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:2116
> +                action = PolicyAction::Suspend;

I might be misunderstanding, but if we're re-using the current process wouldn't we want the action we send back to that process to be Use rather than Suspend?

[Ross Kirsling]

This patch causes WinCairo to crash on launch (of DRT or MiniBrowser).

[Brady Eidson]

(In reply to Ross Kirsling from comment #12)
> This patch causes WinCairo to crash on launch (of DRT or MiniBrowser).

There's no way to diagnose why or what the solution will be without a backtrace.

[Brady Eidson]

(In reply to Ali Juma from comment #11)
> Comment on attachment 340151 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340151&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:2116
> > +                action = PolicyAction::Suspend;
> 
> I might be misunderstanding, but if we're re-using the current process
> wouldn't we want the action we send back to that process to be Use rather
> than Suspend?

You're right. This happened to work through a series of happy accidents, but passing along the existing policy is correct.

[Ross Kirsling]

(In reply to Brady Eidson from comment #13)
> (In reply to Ross Kirsling from comment #12)
> > This patch causes WinCairo to crash on launch (of DRT or MiniBrowser).
> 
> There's no way to diagnose why or what the solution will be without a
> backtrace.

Right, I'm running a debug build to investigate right now. Just wasn't sure whether I should rollout, so I started by commenting instead. 😅

[Brady Eidson]

(In reply to Ross Kirsling from comment #15)
> (In reply to Brady Eidson from comment #13)
> > (In reply to Ross Kirsling from comment #12)
> > > This patch causes WinCairo to crash on launch (of DRT or MiniBrowser).
> > 
> > There's no way to diagnose why or what the solution will be without a
> > backtrace.
> 
> Right, I'm running a debug build to investigate right now. Just wasn't sure
> whether I should rollout, so I started by commenting instead. 😅

It's generally not okay to rollout something that only affects one platform *and* there's not even bot coverage of said platform.

I'm sure it will be easy to resolve, whatever it is - This was not a broad reaching change, and most of it is for a code path that is not enabled by default.

[Brady Eidson]

Reopening to attach new patch.

[Brady Eidson]

Created attachment 340224 [details]
Patch

[Ross Kirsling]

(In reply to Brady Eidson from comment #16)
> (In reply to Ross Kirsling from comment #15)
> > (In reply to Brady Eidson from comment #13)
> > > (In reply to Ross Kirsling from comment #12)
> > > > This patch causes WinCairo to crash on launch (of DRT or MiniBrowser).
> > > 
> > > There's no way to diagnose why or what the solution will be without a
> > > backtrace.
> > 
> > Right, I'm running a debug build to investigate right now. Just wasn't sure
> > whether I should rollout, so I started by commenting instead. 😅
> 
> It's generally not okay to rollout something that only affects one platform
> *and* there's not even bot coverage of said platform.
> 
> I'm sure it will be easy to resolve, whatever it is - This was not a broad
> reaching change, and most of it is for a code path that is not enabled by
> default.

(In reply to Brady Eidson from comment #16)
> (In reply to Ross Kirsling from comment #15)
> > (In reply to Brady Eidson from comment #13)
> > > (In reply to Ross Kirsling from comment #12)
> > > > This patch causes WinCairo to crash on launch (of DRT or MiniBrowser).
> > > 
> > > There's no way to diagnose why or what the solution will be without a
> > > backtrace.
> > 
> > Right, I'm running a debug build to investigate right now. Just wasn't sure
> > whether I should rollout, so I started by commenting instead. 😅
> 
> It's generally not okay to rollout something that only affects one platform
> *and* there's not even bot coverage of said platform.
> 
> I'm sure it will be easy to resolve, whatever it is - This was not a broad
> reaching change, and most of it is for a code path that is not enabled by
> default.

One way or another, it didn't seem like a great way to win friends, haha. 
Thankfully though, the bots are almost working! 😄

That's actually how I discovered the problem (in spite of not having a local build ready to go):
https://build.webkit.org/builders/WinCairo%2064-bit%20WKL%20Release%20%28Tests%29/builds/81/steps/layout-test/logs/stdio

[WebKit Commit Bot]

Comment on attachment 340224 [details]
Patch

Clearing flags on attachment: 340224

Committed r231723: <https://trac.webkit.org/changeset/231723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #19)
> One way or another, it didn't seem like a great way to win friends, haha. 
> Thankfully though, the bots are almost working! 😄
> 
> That's actually how I discovered the problem (in spite of not having a local
> build ready to go):
> https://build.webkit.org/builders/WinCairo%2064-
> bit%20WKL%20Release%20%28Tests%29/builds/81/steps/layout-test/logs/stdio

Evidently, this crash is some sort of incremental build issue for which r231704 serves as a good repro case. I've created a new bug to track this: 
https://bugs.webkit.org/show_bug.cgi?id=185632