Bug 192612 – HTTPS Upgrade: Scripts / preprocessing necessary to create new database in future

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Vivek Seth]

<rdar://problem/45851614>

[Radar WebKit Bug Importer]

<rdar://problem/46651207>

[Vivek Seth]

Created attachment 357101 [details]
Patch

[Vivek Seth]

This is the script used in my new build stage: 

```
# This script requires that HTTPSUpgradeList.txt has the following format:
#   1. It must be a plain text file with domains delimited by new lines ("\n").
#   2. The file must not contain duplicate domains.
#   3. All domains must be lowercase.
#   4. All domains must be IDNA encoded.

set -e

# Do not create database if feature flag is disabled. 
if [[ -z "${ENABLE_HTTPS_UPGRADE}" ]]; then
    exit 0
fi

WK_ADDITIONS_PATH="usr/local/include/WebKitAdditions"

INPUT_FILE_PATH="${BUILT_PRODUCTS_DIR}/${WK_ADDITIONS_PATH}/HTTPSUpgradeList.txt"
OUTPUT_FILE_PATH="${BUILT_PRODUCTS_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/HTTPSUpgradeList.db"

DB_VERSION="1";
DB_SCHEMA="CREATE TABLE hosts (host TEXT PRIMARY KEY);"

# Return early if we don't have HTTPSUpgradeList.txt
if [[ ! -f "${INPUT_FILE_PATH}" ]]; then
    exit 0
fi

# Only create database for macOS and iOS.
if [[ "${WK_PLATFORM_NAME}" != "macosx" && "${WK_PLATFORM_NAME}" != "iphoneos" ]]; then
    exit 0
fi

# Delete database if it exists.
if [[ -f "${OUTPUT_FILE_PATH}" ]]; then   
    rm "${OUTPUT_FILE_PATH}"
fi

# Create database.
sqlite3 "${OUTPUT_FILE_PATH}" "PRAGMA user_version=${DB_VERSION}" "${DB_SCHEMA}" ".import ${INPUT_FILE_PATH} hosts" ".exit"
```

[Vivek Seth]

Comment on attachment 357101 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10447
> +			shellScript = "# This script requires that HTTPSUpgradeList.txt has the following format:\n#   1. It must be a plain text file with domains delimited by new lines (\"\\n\").\n#   2. The file must not contain duplicate domains.\n#   3. All domains must be lowercase.\n#   4. All domains must be IDNA encoded.\n\nset -e\n\n# Do not create database if feature flag is disabled. \nif [[ -z \"${ENABLE_HTTPS_UPGRADE}\" ]]; then\n    exit 0\nfi\n\nWK_ADDITIONS_PATH=\"usr/local/include/WebKitAdditions\"\n\nINPUT_FILE_PATH=\"${BUILT_PRODUCTS_DIR}/${WK_ADDITIONS_PATH}/HTTPSUpgradeList.txt\"\nOUTPUT_FILE_PATH=\"${BUILT_PRODUCTS_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/HTTPSUpgradeList.db\"\n\nDB_VERSION=\"1\";\nDB_SCHEMA=\"CREATE TABLE hosts (host TEXT PRIMARY KEY);\"\n\n# Return early if we don't have HTTPSUpgradeList.txt\nif [[ ! -f \"${INPUT_FILE_PATH}\" ]]; then\n    exit 0\nfi\n\n# Only create database for macOS and iOS.\nif [[ \"${WK_PLATFORM_NAME}\" != \"macosx\" && \"${WK_PLATFORM_NAME}\" != \"iphoneos\" ]]; then\n    exit 0\nfi\n\n# Delete database if it exists.\nif [[ -f \"${OUTPUT_FILE_PATH}\" ]]; then   \n    rm \"${OUTPUT_FILE_PATH}\"\nfi\n\n# Create database.\nsqlite3 \"${OUTPUT_FILE_PATH}\" \"PRAGMA user_version=${DB_VERSION}\" \"${DB_SCHEMA}\" \".import ${INPUT_FILE_PATH} hosts\" \".exit\"\n";

This is the script: 

```
# This script requires that HTTPSUpgradeList.txt has the following format:
#   1. It must be a plain text file with domains delimited by new lines ("\n").
#   2. The file must not contain duplicate domains.
#   3. All domains must be lowercase.
#   4. All domains must be IDNA encoded.

set -e

# Do not create database if feature flag is disabled. 
if [[ -z "${ENABLE_HTTPS_UPGRADE}" ]]; then
    exit 0
fi

WK_ADDITIONS_PATH="usr/local/include/WebKitAdditions"

INPUT_FILE_PATH="${BUILT_PRODUCTS_DIR}/${WK_ADDITIONS_PATH}/HTTPSUpgradeList.txt"
OUTPUT_FILE_PATH="${BUILT_PRODUCTS_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/HTTPSUpgradeList.db"

DB_VERSION="1";
DB_SCHEMA="CREATE TABLE hosts (host TEXT PRIMARY KEY);"

# Return early if we don't have HTTPSUpgradeList.txt
if [[ ! -f "${INPUT_FILE_PATH}" ]]; then
    exit 0
fi

# Only create database for macOS and iOS.
if [[ "${WK_PLATFORM_NAME}" != "macosx" && "${WK_PLATFORM_NAME}" != "iphoneos" ]]; then
    exit 0
fi

# Delete database if it exists.
if [[ -f "${OUTPUT_FILE_PATH}" ]]; then   
    rm "${OUTPUT_FILE_PATH}"
fi

# Create database.
sqlite3 "${OUTPUT_FILE_PATH}" "PRAGMA user_version=${DB_VERSION}" "${DB_SCHEMA}" ".import ${INPUT_FILE_PATH} hosts" ".exit"
```

[Vivek Seth]

Created attachment 357184 [details]
Patch

[Andy Estes]

(In reply to Vivek Seth from comment #4)
> Comment on attachment 357101 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357101&action=review
> 
> This is the script: 

I'd recommend moving the script into its own file in Source/WebKit/Scripts/. Scripts in Xcode project files are hard to deal with, as you can probably see (but thank you for providing it in a comment).

> 
> ```
> # This script requires that HTTPSUpgradeList.txt has the following format:
> #   1. It must be a plain text file with domains delimited by new lines
> ("\n").
> #   2. The file must not contain duplicate domains.
> #   3. All domains must be lowercase.
> #   4. All domains must be IDNA encoded.
> 
> set -e
> 
> # Do not create database if feature flag is disabled. 
> if [[ -z "${ENABLE_HTTPS_UPGRADE}" ]]; then
>     exit 0
> fi
> 
> WK_ADDITIONS_PATH="usr/local/include/WebKitAdditions"
> 
> INPUT_FILE_PATH="${BUILT_PRODUCTS_DIR}/${WK_ADDITIONS_PATH}/HTTPSUpgradeList.
> txt"

This won't work for Production builds. In that configuration, HTTPSUpgradeList.txt will be in $SDKROOT but not $BUILT_PRODUCTS_DIR.

Usually what we do is search for files first in $BUILT_PRODUCTS_DIR/usr/local/include/WebKitAdditions and then in $SDKROOT/usr/local/include/WebKitAdditions. It's easy to do this in makefiles using vpath (see below regarding makefiles).

> OUTPUT_FILE_PATH="${BUILT_PRODUCTS_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH}/
> HTTPSUpgradeList.db"

$BUILT_PRODUCTS_DIR/DerivedSources/WebKit2/ is the usual place WebKit puts generated files.

> 
> # Only create database for macOS and iOS.
> if [[ "${WK_PLATFORM_NAME}" != "macosx" && "${WK_PLATFORM_NAME}" !=
> "iphoneos" ]]; then
>     exit 0
> fi

Why only these platforms?

> 
> # Delete database if it exists.
> if [[ -f "${OUTPUT_FILE_PATH}" ]]; then   
>     rm "${OUTPUT_FILE_PATH}"
> fi
> 
> # Create database.
> sqlite3 "${OUTPUT_FILE_PATH}" "PRAGMA user_version=${DB_VERSION}"
> "${DB_SCHEMA}" ".import ${INPUT_FILE_PATH} hosts" ".exit"
> ```

You haven't specified any input or output paths in Xcode, so I think this phase will re-create the database on every incremental build. That's not great.

Usually we solve this problem with a Makefile. You should teach Source/WebKit/DerivedSources.make how to generate this database so that we only rebuild it when necessary.

[Vivek Seth]

Created attachment 357259 [details]
Patch

[Vivek Seth]

@Andy

> Why only these platforms?

We are starting out with just iOS and macOS and may expand to watchOS in the future. Currently, we cannot support watchOS for performance/disk-space reasons.

[Vivek Seth]

Created attachment 357267 [details]
Patch

[Andy Estes]

Comment on attachment 357259 [details]
Patch

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

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:403
> +ENABLE_HTTPS_UPGRADE = ;

This is the same as just not defining ENABLE_HTTPS_UPGRADE. You shouldn't need to change any FeatureDefines files until this feature will be at least conditionally enabled somewhere.

> Source/WebKit/Configurations/WebKit.xcconfig:155
> -EXCLUDED_SOURCE_FILE_NAMES = Resources/ios/*;
> +EXCLUDED_SOURCE_FILE_NAMES_BASE_ = Resources/ios/* ${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit2/HTTPSUpgradeList.db;
> +EXCLUDED_SOURCE_FILE_NAMES_BASE_ENABLE_HTTPS_UPGRADE = Resources/ios/*;
> +
> +EXCLUDED_SOURCE_FILE_NAMES = $(EXCLUDED_SOURCE_FILE_NAMES_BASE_$(ENABLE_HTTPS_UPGRADE));

We can avoid repeating the "Resources/ios/*" part by splitting things up a little differently:

WK_EXCLUDED_HTTPS_UPGRADE_FILE = $(WK_EXCLUDED_HTTPS_UPGRADE_FILE_$(ENABLE_HTTPS_UPGRADE));
WK_EXCLUDED_HTTPS_UPGRADE_FILE_ = HTTPSUpgradeList.db;

EXCLUDED_SOURCE_FILE_NAMES = Resources/ios/* $(WK_EXCLUDED_HTTPS_UPGRADE_FILE);

(I *think* you can specify HTTPSUpgradeList.db without its path as long as it's included in the project).

> Source/WebKit/DerivedSources.make:320
> +HTTPSUpgradeList.db : $(WebKit2)/Scripts/generate-https-upgrade-database.sh

HTTPSUpgradeList.txt also needs to be a dependency here (as well as the script) because HTTPSUpgradeList.db needs to rebuild when it changes. See below about using VPATH to make this easier.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:24
> +RELATIVE_SOURCE_PATH="usr/local/include/WebKitAdditions"
> +SOURCE_PATH="${BUILT_PRODUCTS_DIR}/${RELATIVE_SOURCE_PATH}"
> +
> +if [[ ! -d "$SOURCE_PATH" ]]; then
> +  SOURCE_PATH="${SDK_DIR}/${RELATIVE_SOURCE_PATH}"
> +fi
> +
> +INPUT_FILE_PATH="${SOURCE_PATH}/HTTPSUpgradeList.txt"

I don't think you need all this. Note that DerivedSources.make has $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) in its VPATH, so it's set up to search $SDKROOT and $BUILT_PRODUCTS_DIR in the right order. I'd just have the makefile find HTTPSUpgradeList.txt in the VPATH then pass that path to the script as a command-line argument.

[Vivek Seth]

Created attachment 357362 [details]
Patch

[Vivek Seth]

> I don't think you need all this. Note that DerivedSources.make has $(WEBKITADDITIONS_HEADER_SEARCH_PATHS) in its VPATH, so it's set up to search $SDKROOT and $BUILT_PRODUCTS_DIR in the right order. I'd just have the makefile find HTTPSUpgradeList.txt in the VPATH then pass that path to the script as a command-line argument.

The reason I had it the other way was that WebKit was failing to build if HTTPSUpgradeList.txt did not exist (which is always true for external users). 

I fixed the issue by moving the conditional checks from my bash script to the Makefile. That way, both internal and external users can build webkit using ($ build-webkit) whether the feature flag is enabled or disabled.

[Andy Estes]

Comment on attachment 357362 [details]
Patch

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

> Source/WebKit/DerivedSources.make:337
> +ifeq ($(USE_INTERNAL_SDK),YES)
> +	ifeq ($(ENABLE_HTTPS_UPGRADE),ENABLE_HTTPS_UPGRADE)
> +
> +		ifeq ($(WK_PLATFORM_NAME),macosx)
> +			WK_CREATE_HTTPS_UPGRADE_DATABASE = YES
> +		endif # WK_PLATFORM_NAME
> +
> +		ifeq ($(WK_PLATFORM_NAME),iphoneos)
> +			WK_CREATE_HTTPS_UPGRADE_DATABASE = YES
> +		endif # WK_PLATFORM_NAME
> +	endif # ENABLE_HTTPS_UPGRADE
> +endif # USE_INTERNAL_SDK
> +
> +ifeq ($(WK_CREATE_HTTPS_UPGRADE_DATABASE),YES)

This seems too complicated. Usually we'd use the values of WK_PLATFORM_NAME and USE_INTERNAL_SDK to set the value of ENABLE_HTTPS_UPGRADE in FeatureDefines.xcconfig and/or FeatureDefines.h. If we did that, then here you'd only need to check if $(ENABLE_HTTPS_UPGRADE) equals ENABLE_HTTPS_UPGRADE.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:1
> +# This script requires that HTTPSUpgradeList.txt has the following format:

You should include a copyright and license at the top of this file.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:18
> +# Return early if we don't have HTTPSUpgradeList.txt.

This comment seems unnecessary (we typically only write "why" comments, not "what" comments).

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:24
> +# Delete database if it exists.

Ditto.

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:29
> +# Create database.

Ditto.

[Andy Estes]

Comment on attachment 357362 [details]
Patch

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

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:3405
> +		587743A421C30AD800AE9084 /* HTTPSUpgradeList.db */ = {isa = PBXFileReference; lastKnownFileType = file; name = HTTPSUpgradeList.db; path = DerivedSources/WebKit2/HTTPSUpgradeList.db; sourceTree = BUILT_PRODUCTS_DIR; };

Something seems wrong here. Usually sourceTree would be "<group>" and path would just contain the file name. There's a way to fix this in the Xcode UI (or I think you could fix it by hand).

[Vivek Seth]

Comment on attachment 357362 [details]
Patch

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

>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:3405
>> +		587743A421C30AD800AE9084 /* HTTPSUpgradeList.db */ = {isa = PBXFileReference; lastKnownFileType = file; name = HTTPSUpgradeList.db; path = DerivedSources/WebKit2/HTTPSUpgradeList.db; sourceTree = BUILT_PRODUCTS_DIR; };
> 
> Something seems wrong here. Usually sourceTree would be "<group>" and path would just contain the file name. There's a way to fix this in the Xcode UI (or I think you could fix it by hand).

This matches how other files are in the DerivedSources folder. For example, see NetworkSocketStreamMessageReceiver.cpp  just below this line.

[Andy Estes]

Comment on attachment 357362 [details]
Patch

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

>>> Source/WebKit/WebKit.xcodeproj/project.pbxproj:3405
>>> +		587743A421C30AD800AE9084 /* HTTPSUpgradeList.db */ = {isa = PBXFileReference; lastKnownFileType = file; name = HTTPSUpgradeList.db; path = DerivedSources/WebKit2/HTTPSUpgradeList.db; sourceTree = BUILT_PRODUCTS_DIR; };
>> 
>> Something seems wrong here. Usually sourceTree would be "<group>" and path would just contain the file name. There's a way to fix this in the Xcode UI (or I think you could fix it by hand).
> 
> This matches how other files are in the DerivedSources folder. For example, see NetworkSocketStreamMessageReceiver.cpp  just below this line.

Oh I see. Well, it matches some but not all other files in DerivedSources. Since the file's group (Derived Sources) is relative to BUILT_PRODUCTS_DIR, I think it's cleaner to make this file be relative to "<group>" and then have the path just be the file name.

Anyway, since we aren't consistent here then this is ok as-is.

[Andy Estes]

Comment on attachment 357362 [details]
Patch

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

> Source/WebKit/Scripts/generate-https-upgrade-database.sh:30
> +sqlite3 "${OUTPUT_FILE_PATH}" "PRAGMA user_version=${DB_VERSION}" "${DB_SCHEMA}" ".import ${INPUT_FILE_PATH} hosts" ".exit"

What permissions do $OUTPUT_FILE_PATH end up with?

[Vivek Seth]

$ ls -lah reports: 

-rw-r--r--  1 vivekseth  admin   160M Dec 17 18:19 ./DerivedData/Safari/Build/Products/Debug/WebKit.framework/Versions/A/Resources/HTTPSUpgradeList.db

This matches other Resources in WebKit.framework. For example:

-rw-r--r--   1 root  wheel    28K Nov 29 21:41 Assets.car

[Vivek Seth]

Created attachment 357589 [details]
Patch

[Vivek Seth]

Comment on attachment 357589 [details]
Patch

Andy already approved this change.

[WebKit Commit Bot]

Comment on attachment 357589 [details]
Patch

Clearing flags on attachment: 357589

Committed r239348: <https://trac.webkit.org/changeset/239348>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.