Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 514859 (webkit2NotRefWebkit1)

Summary: [Webkit2] Verify that no webkit1-only functions are called on Webkit2.
Product: [Eclipse Project] Platform Reporter: Leo Ufimtsev <lufimtse>
Component: SWTAssignee: Leo Ufimtsev <lufimtse>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: P3 CC: akurtakov, ericwill, platform-swt-inbox
Version: 4.7   
Target Milestone: ---   
Hardware: PC   
OS: Linux   
See Also: https://bugs.eclipse.org/bugs/show_bug.cgi?id=522181
https://git.eclipse.org/r/106249
https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=61b1bc783bfeed2794539b90633fbaf9ac64819e
https://git.eclipse.org/r/112731
https://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=7eaf827af2dc3d5e192450bf89a96b1e117bd60d
Whiteboard:
Bug Depends on: 527564    
Bug Blocks: 441568, 516838    

Description Leo Ufimtsev CLA 2017-04-06 09:57:02 EDT 
Currently if webkit2 calls a webkit1-only function, it silently returns a 0.

This causes silent bugs like:
Bug 514719 – [Browser][Webkit2] port Browser.getText() to webkit2 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=514719

I need to go through all the Webkitgtk.java functions and make sure that webkit1-only functions are not called on Webkit2.

Thoughts about implementation:
 || Add assert !isWebkit2, run jUnits & eclipse with assert enabled. Look for asserts being triggered. 
 || Add jni code that throws a warning is when an fp is 0 from trying to get a function.
 || Manual code inspection, see which paths are taken by webkit2 and which by webkit1
Comment 1 Leo Ufimtsev CLA 2017-09-13 11:28:45 EDT 
Calls to webkit1 introduce subtle bugs/crashes. e.g:
Bug 522181 – [GTK3][Webkit2] Webkit1 function webkit_get_default_session is called on Webkit2, causing a crash

As such, it is important to complete this task before webkit2 port is marked complete.
Comment 2 Leo Ufimtsev CLA 2017-10-03 17:46:25 EDT 
WebkitGtk 1/2 index specifies which functions are available in webkit 1 & 2:

Webkit1 functions:
https://webkitgtk.org/reference/webkitgtk/unstable/   -> 'index'

Webkit2 functions:
https://webkitgtk.org/reference/webkit2gtk/unstable/  -> 'index'


1) So for all functions in WebKitGTK.java, we'd need to see if a function is webkit1, webkit2 or available in both. (This step can potentially be done faster via scripting?)

2) For webkit1 only functions, check that Webkit2 never reaches it. (probably manual work)

3) It would be beneficial to add assertions into WebKitGTK.java. I.e, 
- for webkit1-only functions, assert that it's ran by webkit1
- for webkit2-only functions, assert that it's ran by webkit2
- for webkit 1 & 2 functions, ~maybe assert that either webkit1/2 are ran or just leave a comment. 
Note, WebkitGTK.java doesn't have access to "WEBKIT2" variable. To implement asserts either variable/LoadLibrary() should be moved to WebKitGTK.java or WEBKIT2 variable should be made public and @noreference.
Comment 3 Eclipse Genie CLA 2017-10-04 19:02:37 EDT 
New Gerrit change created: https://git.eclipse.org/r/106249
Comment 4 Leo Ufimtsev CLA 2017-10-04 19:12:54 EDT 
(In reply to Leo Ufimtsev from comment #2)
> WebkitGtk 1/2 index specifies which functions are available in webkit 1 & 2:
> 
> Webkit1 functions:
> https://webkitgtk.org/reference/webkitgtk/unstable/   -> 'index'
> 
> Webkit2 functions:
> https://webkitgtk.org/reference/webkit2gtk/unstable/  -> 'index'
> 
> 
> 1) So for all functions in WebKitGTK.java, we'd need to see if a function is
> webkit1, webkit2 or available in both. (This step can potentially be done
> faster via scripting?)
> 
> 2) For webkit1 only functions, check that Webkit2 never reaches it.
> (probably manual work)
> 
> 3) It would be beneficial to add assertions into WebKitGTK.java. I.e, 
> - for webkit1-only functions, assert that it's ran by webkit1
> - for webkit2-only functions, assert that it's ran by webkit2
> - for webkit 1 & 2 functions, ~maybe assert that either webkit1/2 are ran or
> just leave a comment. 
> Note, WebkitGTK.java doesn't have access to "WEBKIT2" variable. To implement
> asserts either variable/LoadLibrary() should be moved to WebKitGTK.java or
> WEBKIT2 variable should be made public and @noreference.

I implemented the above. I found a bunch of webkit1 only functions that are reached by webkit2. They seem to be related to downloading content.

To name a few:
webkit_download_get_current_size
webkit_download_get_total_size
webkit_download_get_network_request
webkit_download_get_status
webkit_download_get_suggested_filename
webkit_download_get_total_size
webkit_download_get_uri
webkit_download_new
... see '//TODO - guard from being called on webkit2.' in patch https://git.eclipse.org/r/#/c/106249/
Comment 6 Leo Ufimtsev CLA 2017-11-06 15:09:15 EST 
Outstanding tasks:
[] Ensure all signals in WebkitGTK.java are commented as "// Webkit (1|2)".
   This is useful when looking for minimum required version of webkitgtk and making sure webkit1/2 signals are not mixed. 
[] When doing above, double check that signals are matched up with correct handlers. Some signals exist in both webkit1 and webkit2 but have different callback signatures. e.g 'context-menu' signal.
Comment 7 Eclipse Genie CLA 2017-12-01 14:45:29 EST 
New Gerrit change created: https://git.eclipse.org/r/112731
Comment 8 Leo Ufimtsev CLA 2017-12-01 14:47:17 EST 
Checked that signals are properly ported and documented:
https://git.eclipse.org/r/#/c/112731/

Awaiting parent patches to be merged to avoid merge conflicts. Then this bug can be closed.