From 52b34c645e8dbf90a43e3c7985f9c9bbe4d11918 Mon Sep 17 00:00:00 2001
From: Justin Cohen <justincohen@google.com>
Date: Wed, 14 Jan 2026 06:15:19 -0800
Subject: [PATCH] Reland "net: Refactor GURLWithNSURL to fix 'about:' scheme percent-encoding"

This CL introduces a kill switch feature kUseNSURLDataForGURLConversion
to control the logic in GURLWithNSURL. When enabled, the conversion
logic prefers [NSURL dataRepresentation] over [NSURL absoluteString] if
they differ with the `about` scheme URLs. The dataRepresentation of the
NSURL is the raw bytes of the URL pre-canonicalization, so the approach
tries to favor GURL's interpretation over NSURL.

This change aims to address issues where absoluteString returns a
percent-encoded string (e.g., about:blank%23hash) while
dataRepresentation returns the raw bytes (e.g., about:blank#hash), which
is the desired input for GURL.

A new histogram Net.Apple.NSURL.DataMismatch is recorded when the two
representations differ, helping to monitor the impact of this change.
Net.Apple.NSURL.DataMismatch.Scheme is recorded to identify which
schemes are affected, if this happens beyond `about`

Reland changes in patchset 2
  Add UseNSURLDataForGURLConversion to AppShimController to allow-list
  it to be checked in early startup of PWAs.

Include-Ci-Only-Tests: chromium.mac:Mac13 Tests|browser_tests
Bug: 40932726, 470295118, 474953367
Change-Id: I95930c7d299c9f1f04ec65868fcba2e4c8bd0bd1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7464958
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Auto-Submit: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Nick Harper <nharper@chromium.org>
Reviewed-by: Hayato Ito <hayato@chromium.org>
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1569077}
---

diff --git a/chrome/app_shim/app_shim_controller.mm b/chrome/app_shim/app_shim_controller.mm
index de636b14..c3a78a7 100644
--- a/chrome/app_shim/app_shim_controller.mm
+++ b/chrome/app_shim/app_shim_controller.mm
@@ -293,6 +293,7 @@
        "UseIDNAContextJRules", "MojoBindingsInlineSLS",
        "MojoInlineMessagePayloads", "MojoIpcz", "MojoIpczMemV2",
        "MojoFixGeometricBufferGrowth", "UseAdHocSigningForWebAppShims",
+       "UseNSURLDataForGURLConversion",
        "SonomaAccessibilityActivationRefinements", "FeatureParamWithCache",
        "UseMachVouchers"});
 }
diff --git a/ios/chrome/browser/web/model/window_open_by_dom_egtest.mm b/ios/chrome/browser/web/model/window_open_by_dom_egtest.mm
index d59a009..fd89966 100644
--- a/ios/chrome/browser/web/model/window_open_by_dom_egtest.mm
+++ b/ios/chrome/browser/web/model/window_open_by_dom_egtest.mm
@@ -175,23 +175,15 @@
   [ChromeEarlGrey tapWebStateElementWithID:
                       @"webScenarioLocationReplaceInWindowOpenWithEmptyTarget"];
   [ChromeEarlGrey waitForMainTabCount:2];
-  // WebKit doesn't parse 'about:blank#hash' as about:blank with URL fragment.
-  // Instead, it percent encodes '#hash' and considers 'blank%23hash' as the
-  // resource identifier. Nevertheless, the '#' is significant in triggering the
-  // edge case in the bug. TODO(crbug.com/41414501): Change back to '#'.
-  // Since about scheme URLs are also trimmed to about:blank, check the url
-  // directly instead.
-  //
-  // TODO(crbug.com/40932726): Confirm the expected behavior of [ChromeEarlGrey
-  // webStateLastCommittedURL] here. After https://crrev.com/c/4823237, this
-  // returns empty URL ("").
-  DCHECK_EQ("", [ChromeEarlGrey webStateLastCommittedURL]);
-  // And confirm the location bar and focused omnibox are empty.
+  DCHECK_EQ(GURL("about:blank#hash"),
+            [ChromeEarlGrey webStateLastCommittedURL]);
+  // And confirm the location bar shows "about:blank".
   [[EarlGrey selectElementWithMatcher:chrome_test_util::DefocusedLocationView()]
-      assertWithMatcher:chrome_test_util::LocationViewEmpty()];
+      assertWithMatcher:chrome_test_util::LocationViewContainingText(
+                            "about:blank")];
   [ChromeEarlGreyUI focusOmnibox];
   [[EarlGrey selectElementWithMatcher:chrome_test_util::Omnibox()]
-      assertWithMatcher:chrome_test_util::OmniboxText("")];
+      assertWithMatcher:chrome_test_util::OmniboxText("about:blank")];
   [OmniboxEarlGrey defocusOmnibox];
 }
 
diff --git a/net/BUILD.gn b/net/BUILD.gn
index ee245ae..669bb5f 100644
--- a/net/BUILD.gn
+++ b/net/BUILD.gn
@@ -4303,6 +4303,17 @@
   ]
 }
 
+if (is_apple) {
+  fuzzer_test("net_url_conversions_fuzzer") {
+    sources = [ "base/apple/url_conversions_fuzzer.mm" ]
+    deps = [
+      "//base",
+      "//net",
+    ]
+    frameworks = [ "Foundation.framework" ]
+  }
+}
+
 if (is_linux || is_chromeos) {
   fuzzer_test("net_base_address_tracker_linux_fuzzer") {
     sources = [ "base/address_tracker_linux_fuzzer.cc" ]
diff --git a/net/base/apple/url_conversions.mm b/net/base/apple/url_conversions.mm
index 8f38979..16481f1 100644
--- a/net/base/apple/url_conversions.mm
+++ b/net/base/apple/url_conversions.mm
@@ -6,10 +6,64 @@
 
 #import <Foundation/Foundation.h>
 
+#include "base/containers/fixed_flat_map.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/escape.h"
+#include "base/strings/sys_string_conversions.h"
+#include "net/base/features.h"
 #include "url/gurl.h"
 #include "url/url_canon.h"
 
+namespace {
+
+// Schemes that are tracked in the Net.Apple.NSURL.DataMismatch.Scheme
+// histogram. These values are persisted to logs. Entries should not be
+// renumbered and numeric values should never be reused.
+enum class Scheme {
+  kUnknown = 0,
+  kOther = 1,
+  kAbout = 2,
+  kBlob = 3,
+  kContent = 4,
+  kData = 5,
+  kFile = 6,
+  kFileSystem = 7,
+  kFtp = 8,
+  kHttp = 9,
+  kHttps = 10,
+  kMailto = 11,
+  kTel = 12,
+  kMaxValue = kTel,
+};
+
+Scheme SchemeForURL(NSURL* url) {
+  NSString* scheme = [url scheme];
+  if (!scheme) {
+    return Scheme::kUnknown;
+  }
+
+  static constexpr auto kSchemeMap =
+      base::MakeFixedFlatMap<std::string_view, Scheme>({
+          {"about", Scheme::kAbout},
+          {"blob", Scheme::kBlob},
+          {"content", Scheme::kContent},
+          {"data", Scheme::kData},
+          {"file", Scheme::kFile},
+          {"filesystem", Scheme::kFileSystem},
+          {"ftp", Scheme::kFtp},
+          {"http", Scheme::kHttp},
+          {"https", Scheme::kHttps},
+          {"mailto", Scheme::kMailto},
+          {"tel", Scheme::kTel},
+      });
+
+  std::string lower_scheme = base::SysNSStringToUTF8([scheme lowercaseString]);
+  auto it = kSchemeMap.find(lower_scheme);
+  return it != kSchemeMap.end() ? it->second : Scheme::kOther;
+}
+
+}  // namespace
+
 namespace net {
 
 NSURL* NSURLWithGURL(const GURL& url) {
@@ -45,10 +99,41 @@
 }
 
 GURL GURLWithNSURL(NSURL* url) {
-  if (url) {
-    return GURL(url.absoluteString.UTF8String);
+  if (!url) {
+    return GURL();
   }
-  return GURL();
+
+  std::string_view standard_url_string(url.absoluteString.UTF8String);
+
+  // Foundation sometimes encodes the URL in absoluteString (and all the
+  // NSURL accessors other than dataRepresentation), which is not what we want
+  // to pass to GURL. For example, 'about:blank#hash' becomes
+  // 'about:blank%23hash' in absoluteString, but remains 'about:blank#hash' in
+  // dataRepresentation.
+  if (base::FeatureList::IsEnabled(features::kUseNSURLDataForGURLConversion)) {
+    NSData* data = [url dataRepresentation];
+    if (data && data.length > 0) {
+      std::string_view data_url_string(
+          reinterpret_cast<const char*>(data.bytes), data.length);

diff --git a/net/base/apple/url_conversions_unittest.mm b/net/base/apple/url_conversions_unittest.mm
index db0c83a3..180299fc 100644
--- a/net/base/apple/url_conversions_unittest.mm
+++ b/net/base/apple/url_conversions_unittest.mm
@@ -2,10 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#import "net/base/apple/url_conversions.h"
+
 #import <Foundation/Foundation.h>
 
 #include "base/strings/sys_string_conversions.h"
-#import "net/base/apple/url_conversions.h"
+#include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_feature_list.h"
+#include "net/base/features.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gtest_mac.h"
 #include "url/gurl.h"
@@ -234,6 +238,33 @@
   EXPECT_EQ(nil, NSURLWithGURL(url));
 }
 
+TEST_F(URLConversionTest, TestGURLWithNSURLFeatureEnabled) {
+  base::HistogramTester histogram_tester;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(
+      features::kUseNSURLDataForGURLConversion);
+
+  NSURL* url = [NSURL URLWithString:@"about:blank#hash"];
+  GURL gurl = GURLWithNSURL(url);
+  EXPECT_EQ("about:blank#hash", gurl.spec());
+  histogram_tester.ExpectUniqueSample("Net.Apple.NSURL.DataMismatch", true, 1);
+}
+
+TEST_F(URLConversionTest, TestGURLWithNSURLFeatureDisabled) {
+  base::HistogramTester histogram_tester;
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndDisableFeature(
+      features::kUseNSURLDataForGURLConversion);
+
+  NSURL* url = [NSURL URLWithString:@"about:blank#hash"];
+  GURL gurl = GURLWithNSURL(url);
+  // If this test fails, it means Apple has fixed the bug in NSURL's
+  // absoluteString, and the kUseNSURLDataForGURLConversion workaround can be
+  // removed.
+  EXPECT_EQ("about:blank%23hash", gurl.spec());
+  histogram_tester.ExpectTotalCount("Net.Apple.NSURL.DataMismatch", 0);
+}
+
 }  // namespace
 
 }  // namespace net