Do not sanitize about:blank/#foo & about:blank?foo

Do not sanitize about:blank/#foo & about:blank?foo

This CL ensures that the browser will not sanitize these two types of
URLs. This fixes an issue with PlzNavigate where we could not go back to
about:blank/#foo due to some discrepancy between the url in the
FrameNavigationEntry & the url stored in the PageState of the same
entry.

BUG=575210
Review-Url: https://codereview.chromium.org/2644133002
Cr-Commit-Position: refs/heads/master@{#445525}
Committed: https://chromium.googlesource.com/chromium/src/+/eff9252e95930e904f389f7f902bd9c815243f82

[clamy, 2017-01-19 16:32:33]

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 16:33:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-01-19 17:53:10]

Description was changed from

==========
Do not sanitize about:blank/#foo & about:blank?foo

This CL ensures that the browser will not sanitize these two types of
URLs. This fixes an issue with PlzNavigate where we could not go back to
about:blank/#foo due to some discrepancy between the url in the
FrameNavigationEntry & the url stored in the PageState of the same
entry.

BUG=575210
==========

to

==========
Do not sanitize about:blank/#foo & about:blank?foo

This CL ensures that the browser will not sanitize these two types of
URLs. This fixes an issue with PlzNavigate where we could not go back to
about:blank/#foo due to some discrepancy between the url in the
FrameNavigationEntry & the url stored in the PageState of the same
entry.

BUG=575210
==========

[clamy, 2017-01-19 17:53:10]

clamy@chromium.org changed reviewers:
+ creis@chromium.org

[clamy, 2017-01-19 17:54:51]

@creis: PTAL

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
content/public/common/url_constants.h:30: CONTENT_EXPORT extern const char
kAboutBlankPath[];
Note: I've put all this in this file since it seemed to be the only file in
content/ with url constants, even though I'm not using it outside of content/.
If you have a suggestion for a better place to put it that'd be nice.

[commit-bot: I haz the power, 2017-01-19 18:06:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-19 18:06:28]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-01-19 22:36:34]

creis@chromium.org changed reviewers:
+ mkwst@chromium.org

[Charlie Reis, 2017-01-19 22:36:35]

I like this!  Adding mkwst@ in case we want to move the IsAboutBlankURL utility
to url/.

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
content/public/common/url_constants.h:30: CONTENT_EXPORT extern const char
kAboutBlankPath[];
On 2017/01/19 17:54:51, clamy wrote:
> Note: I've put all this in this file since it seemed to be the only file in
> content/ with url constants, even though I'm not using it outside of content/.
> If you have a suggestion for a better place to put it that'd be nice.

I was going to suggest content/common/content_constants_internal.h, but I wonder
if it's better to add an external use in this CL and just keep it public.  That
may help us understand this a bit more as well.

For example, from a quick glance I noticed WillHandleBrowserAboutURL in
chrome/browser/browser_about_handler.cc, which DCHECKs if the URL has an about:
scheme but isn't equal to kAboutBlankURL.  That pointed me to FixupURL in
components/url_formatter/url_fixer.cc, which rewrites any about: URLs to chrome:
URLs unless they're equal to kAboutBlankURL.  And indeed, we seem to have a bug
where you can't type about:blank/#foo into the omnibox!  (Firefox supports this;
Edge does not.)

This isn't high impact to users, but maybe it's worth correcting?  If so, maybe
your IsAboutBlankURL utility and these constants actually belong in
url/url_util.h and url/url_constants.h?  mkwst@ may have thoughts on this.

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_u...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_u...
content/public/common/url_utils.cc:35: if (url.path() != kAboutBlankPath &&
url.path() !=kAboutBlankWithHashPath)
nit: Space after !=

https://codereview.chromium.org/2644133002/diff/1/content/renderer/render_fra...
File content/renderer/render_frame_impl.cc (left):

https://codereview.chromium.org/2644133002/diff/1/content/renderer/render_fra...
content/renderer/render_frame_impl.cc:5476: // about:blank. Don't send these to
the browser.
Awesome.  I love getting rid of checks like this.  :)

[Mike West, 2017-01-20 09:10:37]

On 2017/01/19 at 22:36:35, creis wrote:
> I like this!  Adding mkwst@ in case we want to move the IsAboutBlankURL
utility to url/.
> 
>
https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
> File content/public/common/url_constants.h (right):
> 
>
https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
> content/public/common/url_constants.h:30: CONTENT_EXPORT extern const char
kAboutBlankPath[];
> On 2017/01/19 17:54:51, clamy wrote:
> > Note: I've put all this in this file since it seemed to be the only file in
> > content/ with url constants, even though I'm not using it outside of
content/.
> > If you have a suggestion for a better place to put it that'd be nice.
> 
> I was going to suggest content/common/content_constants_internal.h, but I
wonder if it's better to add an external use in this CL and just keep it public.
 That may help us understand this a bit more as well.
> 
> For example, from a quick glance I noticed WillHandleBrowserAboutURL in
chrome/browser/browser_about_handler.cc, which DCHECKs if the URL has an about:
scheme but isn't equal to kAboutBlankURL.  That pointed me to FixupURL in
components/url_formatter/url_fixer.cc, which rewrites any about: URLs to chrome:
URLs unless they're equal to kAboutBlankURL.  And indeed, we seem to have a bug
where you can't type about:blank/#foo into the omnibox!  (Firefox supports this;
Edge does not.)

I agree with Charlie that this seems like a reasonable thing to add to //url. If
you'd like to move it there in this or a subsequent CL, I'm happy to review.

[clamy, 2017-01-20 12:51:06]

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 12:51:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-01-20 12:51:53]

Thanks! As per suggestions, I moved the util code in url/. PTAL

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
File content/public/common/url_constants.h (right):

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_c...
content/public/common/url_constants.h:30: CONTENT_EXPORT extern const char
kAboutBlankPath[];
On 2017/01/19 22:36:35, Charlie Reis wrote:
> On 2017/01/19 17:54:51, clamy wrote:
> > Note: I've put all this in this file since it seemed to be the only file in
> > content/ with url constants, even though I'm not using it outside of
content/.
> > If you have a suggestion for a better place to put it that'd be nice.
> 
> I was going to suggest content/common/content_constants_internal.h, but I
wonder
> if it's better to add an external use in this CL and just keep it public. 
That
> may help us understand this a bit more as well.
> 
> For example, from a quick glance I noticed WillHandleBrowserAboutURL in
> chrome/browser/browser_about_handler.cc, which DCHECKs if the URL has an
about:
> scheme but isn't equal to kAboutBlankURL.  That pointed me to FixupURL in
> components/url_formatter/url_fixer.cc, which rewrites any about: URLs to
chrome:
> URLs unless they're equal to kAboutBlankURL.  And indeed, we seem to have a
bug
> where you can't type about:blank/#foo into the omnibox!  (Firefox supports
this;
> Edge does not.)
> 
> This isn't high impact to users, but maybe it's worth correcting?  If so,
maybe
> your IsAboutBlankURL utility and these constants actually belong in
> url/url_util.h and url/url_constants.h?  mkwst@ may have thoughts on this.

As per mkwst@ input, I've moved this to url/.

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_u...
File content/public/common/url_utils.cc (right):

https://codereview.chromium.org/2644133002/diff/1/content/public/common/url_u...
content/public/common/url_utils.cc:35: if (url.path() != kAboutBlankPath &&
url.path() !=kAboutBlankWithHashPath)
On 2017/01/19 22:36:35, Charlie Reis wrote:
> nit: Space after !=

Done.

[Mike West, 2017-01-20 12:57:57]

LGTM % tests.

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.cc#newcode623
url/url_util.cc:623: return true;
There's enough logic here that adding tests seems reasonable. Would you mind
putting some into `url_util_unittest.cc`?

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.h#newcode158
url/url_util.h:158: URL_EXPORT bool IsAboutBlankURL(const GURL& url);
Nit: `IsAboutBlank` is shorter, and seems clear enough.

[clamy, 2017-01-20 13:16:52]

The CQ bit was checked by clamy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 13:17:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-01-20 13:19:28]

Thanks!

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.cc
File url/url_util.cc (right):

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.cc#newcode623
url/url_util.cc:623: return true;
On 2017/01/20 12:57:56, Mike West (sloooooow) wrote:
> There's enough logic here that adding tests seems reasonable. Would you mind
> putting some into `url_util_unittest.cc`?

Done. Let me know if you think I should add another test case.

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2644133002/diff/40001/url/url_util.h#newcode158
url/url_util.h:158: URL_EXPORT bool IsAboutBlankURL(const GURL& url);
On 2017/01/20 12:57:57, Mike West (sloooooow) wrote:
> Nit: `IsAboutBlank` is shorter, and seems clear enough.

Done.

[commit-bot: I haz the power, 2017-01-20 14:48:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 14:48:29]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Charlie Reis, 2017-01-20 20:31:02]

Thanks!  LGTM with nits.

Once this lands, we can update WillHandleBrowserAboutURL to get omnibox
navigations to these to work.  (I'm guessing there will be other places that
want to use the new utility as well, but there's probably no rush.)  I can file
a bug for it after the CL lands, unless you'd like to take it.

https://codereview.chromium.org/2644133002/diff/60001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2644133002/diff/60001/url/url_util.h#newcode156
url/url_util.h:156: // Check whether the url is of the form about:blank,
about:blank?foo or
nit: Add blank line before, since this isn't directly related to
GetStandardSchemeType.

https://codereview.chromium.org/2644133002/diff/60001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/2644133002/diff/60001/url/url_util_unittest.c...
url/url_util_unittest.cc:421: const std::string kAboutBlankURLS[] =
{"about:blank", "about:blank?foo",
Minor nit: Lowercase s at the end (kAboutBlankURLs).  Same below.

https://codereview.chromium.org/2644133002/diff/60001/url/url_util_unittest.c...
url/url_util_unittest.cc:429: "about:blank/foo", "about://:8000/blank",
"about://foo:foo@/blank"};
A few more to add:
foo@about:blank
foo:bar@about:blank
about:blank:8000

[clamy, 2017-01-23 12:26:27]

Thanks! Filing a bug sgtm.

https://codereview.chromium.org/2644133002/diff/60001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2644133002/diff/60001/url/url_util.h#newcode156
url/url_util.h:156: // Check whether the url is of the form about:blank,
about:blank?foo or
On 2017/01/20 20:31:02, Charlie Reis wrote:
> nit: Add blank line before, since this isn't directly related to
> GetStandardSchemeType.

Done.

https://codereview.chromium.org/2644133002/diff/60001/url/url_util_unittest.cc
File url/url_util_unittest.cc (right):

https://codereview.chromium.org/2644133002/diff/60001/url/url_util_unittest.c...
url/url_util_unittest.cc:421: const std::string kAboutBlankURLS[] =
{"about:blank", "about:blank?foo",
On 2017/01/20 20:31:02, Charlie Reis wrote:
> Minor nit: Lowercase s at the end (kAboutBlankURLs).  Same below.

Done.

https://codereview.chromium.org/2644133002/diff/60001/url/url_util_unittest.c...
url/url_util_unittest.cc:429: "about:blank/foo", "about://:8000/blank",
"about://foo:foo@/blank"};
On 2017/01/20 20:31:02, Charlie Reis wrote:
> A few more to add:
> foo@about:blank
> foo:bar@about:blank
> about:blank:8000

Done.

[clamy, 2017-01-23 12:26:37]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-01-23 12:26:38]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2644133002/#ps80001
(title: "Rebase + addressed comments")

[commit-bot: I haz the power, 2017-01-23 12:26:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 13:17:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 13:17:10]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[clamy, 2017-01-23 13:39:57]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-01-23 13:39:58]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2644133002/#ps100001
(title: "Try to fix Windows linkage error")

[commit-bot: I haz the power, 2017-01-23 13:40:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 13:50:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 13:50:24]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[clamy, 2017-01-23 13:57:12]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-01-23 13:57:12]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2644133002/#ps120001
(title: "Add missing include")

[commit-bot: I haz the power, 2017-01-23 13:57:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 13:58:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 13:58:18]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, no build
URL)

[clamy, 2017-01-23 14:21:28]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2017-01-23 14:21:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 14:22:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 14:22:33]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[clamy, 2017-01-23 14:24:45]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2017-01-23 14:25:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 14:26:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 14:26:15]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[clamy, 2017-01-23 15:47:16]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2017-01-23 15:47:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 18:00:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 18:00:40]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[clamy, 2017-01-23 18:04:03]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2017-01-23 18:04:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 22:49:03]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1485194643481440,
"parent_rev": "8f64e9220762c7c7d5237cb4b4222898c5b0f4dc", "commit_rev":
"eff9252e95930e904f389f7f902bd9c815243f82"}

[commit-bot: I haz the power, 2017-01-23 22:49:40]

Description was changed from

==========
Do not sanitize about:blank/#foo & about:blank?foo

This CL ensures that the browser will not sanitize these two types of
URLs. This fixes an issue with PlzNavigate where we could not go back to
about:blank/#foo due to some discrepancy between the url in the
FrameNavigationEntry & the url stored in the PageState of the same
entry.

BUG=575210
==========

to

==========
Do not sanitize about:blank/#foo & about:blank?foo

This CL ensures that the browser will not sanitize these two types of
URLs. This fixes an issue with PlzNavigate where we could not go back to
about:blank/#foo due to some discrepancy between the url in the
FrameNavigationEntry & the url stored in the PageState of the same
entry.

BUG=575210

Review-Url: https://codereview.chromium.org/2644133002
Cr-Commit-Position: refs/heads/master@{#445525}
Committed:
https://chromium.googlesource.com/chromium/src/+/eff9252e95930e904f389f7f902b...
==========

[commit-bot: I haz the power, 2017-01-23 22:49:41]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/eff9252e95930e904f389f7f902b...

[brettw, 2017-01-26 21:30:42]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2017-01-26 21:30:43]

https://codereview.chromium.org/2644133002/diff/120001/url/url_util.h
File url/url_util.h (right):

https://codereview.chromium.org/2644133002/diff/120001/url/url_util.h#newcode160
url/url_util.h:160: URL_EXPORT bool IsAboutBlank(const GURL& url);
url_util is not right right place for this function. This is really the
infrastructure that underlies both GURL and KURL, rather than functions that
operate on URLs. This is probably misleading given the name, but you can see
that there are no other functions dealing with GURL here.

Can you remove this function?

On https://codereview.chromium.org/2584513003/ I recommended adding a
"EqualsIgnoringRef function on GURL. Then you can change the constant to just
"about:blank" and the two callers can call EqualsIgnoringRef(kAboutBlankUrl");

I don't think this will be slower than the current implementation. Are there
cases where this implementation wouldn't work?

[clamy, 2017-01-27 12:35:13]

On 2017/01/26 21:30:43, brettw (ping after 24h) wrote:
> https://codereview.chromium.org/2644133002/diff/120001/url/url_util.h
> File url/url_util.h (right):
> 
>
https://codereview.chromium.org/2644133002/diff/120001/url/url_util.h#newcode160
> url/url_util.h:160: URL_EXPORT bool IsAboutBlank(const GURL& url);
> url_util is not right right place for this function. This is really the
> infrastructure that underlies both GURL and KURL, rather than functions that
> operate on URLs. This is probably misleading given the name, but you can see
> that there are no other functions dealing with GURL here.
> 
> Can you remove this function?
> 
> On https://codereview.chromium.org/2584513003/ I recommended adding a
> "EqualsIgnoringRef function on GURL. Then you can change the constant to just
> "about:blank" and the two callers can call EqualsIgnoringRef(kAboutBlankUrl");
> 
> I don't think this will be slower than the current implementation. Are there
> cases where this implementation wouldn't work?

Sure I can move that in a follow up patch when Arthur's patch has landed. I'll
see if EqualsIgnoringRef work for our specific use case and use it if it does.

[brettw, 2017-02-07 05:08:16]

On 2017/01/27 12:35:13, clamy (very slow - traveling) wrote:
> On 2017/01/26 21:30:43, brettw (ping after 24h) wrote:
> > https://codereview.chromium.org/2644133002/diff/120001/url/url_util.h
> > File url/url_util.h (right):
> > 
> >
>
https://codereview.chromium.org/2644133002/diff/120001/url/url_util.h#newcode160
> > url/url_util.h:160: URL_EXPORT bool IsAboutBlank(const GURL& url);
> > url_util is not right right place for this function. This is really the
> > infrastructure that underlies both GURL and KURL, rather than functions that
> > operate on URLs. This is probably misleading given the name, but you can see
> > that there are no other functions dealing with GURL here.
> > 
> > Can you remove this function?
> > 
> > On https://codereview.chromium.org/2584513003/ I recommended adding a
> > "EqualsIgnoringRef function on GURL. Then you can change the constant to
just
> > "about:blank" and the two callers can call
EqualsIgnoringRef(kAboutBlankUrl");
> > 
> > I don't think this will be slower than the current implementation. Are there
> > cases where this implementation wouldn't work?
> 
> Sure I can move that in a follow up patch when Arthur's patch has landed. I'll
> see if EqualsIgnoringRef work for our specific use case and use it if it does.

Thanks!