Support TURN TLS in WebRTC

VERIFIED FIXED in Firefox 53

Status

()

Core
WebRTC: Networking
P1
normal
Rank:
15
VERIFIED FIXED
3 years ago
3 months ago

People

(Reporter: Thomas Bruun, Assigned: bwc, NeedInfo)

Tracking

(Depends on: 1 bug)

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Reporter)

Description

3 years ago
Support TURN TLS in WebRTC.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 949703
Excuse my confusion, bug 949703 is for proxying webrtc tcp traffic, not turn/tls per se.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
FWIW, you'll probably have to adapt the local candidate priority as part of this. 0 for TURN/TCP doesn't leave much space.
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3

Comment 4

2 years ago
I would highly appreciate this feature! In many companies TURN over TCP/TLS would make WebRTC traffic to go through the firewall (on port 443) without additional configuration. Already working with Chrome.

Comment 5

2 years ago
My demo of WebRTC at the Cambridge mini-DebConf failed to work on the wifi in the venue because of this.  Many corporate networks, meeting venues, hotels, etc, have problems with TURN over UDP and they need the user to do TURN over TLS.

I tried explicitly configuring an ICE server using "turns:turn-server.example.org:443" and Firefox just doesn't try to use it at all.  The version I tested was 41.0.1
(Assignee)

Comment 6

2 years ago
Ick, the hotel was interfering with TURN TCP?
(Assignee)

Comment 7

2 years ago
(Is it possible you were running your demo in an e10s window? TURN TCP is still not working in e10s.)

Comment 8

2 years ago
I only had the option of TURN over TLS or TURN over UDP.  I haven't been trying TURN over TCP.

The wifi wasn't operated by the hotel, it was the guest wifi of the tech firm hosting the event.

I worked around it by tethering my laptop to a mobile phone, where TURN over UDP was successful.

I have tried to check TURN over TLS status again today (both in Firefox and Chrome and also in ice4j) and didn't experience great results.  In Chrome it does work, but Chrome has other problems on my Debian system
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=770659

Comment 9

2 years ago
hi,
This feature would be great! working for a big company with webrtc using turn over tls in port 443 trough firewall. Chrome work without any problem, last FF46: TURNS is not yet supported.
Rank: 35 → 25
Priority: P3 → P2
Assignee: nobody → docfaraday
Rank: 25 → 15
Priority: P2 → P1

Comment 10

a year ago
When can expect this issue to be resolved ? Would be great to have this feature to support webrtc call on Firefox for the networks which only supports TLS over port 443.

Comment 11

a year ago
When can expect this issue to be resolved ? Would be great to have this feature to support webrtc call on Firefox for the networks which only supports TLS over port 443.
We're working on it now (Byron is the lead developer), and Firefox 53 is our target.
(Assignee)

Updated

11 months ago
Depends on: 1323439
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8823470 - Flags: review?(drno) → review?(mcmanus)

Comment 20

11 months ago
mozreview-review
Comment on attachment 8823470 [details]
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.

https://reviewboard.mozilla.org/r/101996/#review102572

::: dom/network/TCPSocketParent.cpp:153
(Diff revision 1)
>    nsCOMPtr<nsISocketTransport> socketTransport;
> -  rv = sts->CreateTransport(nullptr, 0,
> +  const char* socketTypes[1];
> +  if (aUseSSL) {
> +    socketTypes[0] = "ssl";
> +  } else {
> +    socketTypes[0] = "starttls";

do you want starttls here, or (nullptr, 0) ?

Comment 21

11 months ago
mozreview-review
Comment on attachment 8819455 [details]
Bug 1056934 - Part 1: TLS support in the test ICE server.

https://reviewboard.mozilla.org/r/99218/#review102768
Attachment #8819455 - Flags: review?(drno) → review+

Comment 22

11 months ago
mozreview-review
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

LGTM

I would like the issues to get resolved. I'll leave it up to you if you want to address comments without issues.

::: media/mtransport/nricectx.cpp:622
(Diff revision 1)
>      MOZ_MTLOG(ML_DEBUG, "NAT mapping type: " << mapping_type.get());
>      TestNat* test_nat = new TestNat;
>      test_nat->filtering_type_ = TestNat::ToNatBehavior(filtering_type.get());
>      test_nat->mapping_type_ = TestNat::ToNatBehavior(mapping_type.get());
>      test_nat->block_udp_ = block_udp;
> +    test_nat->block_stun_ = false;

Is there a reason for over writing the constructor default value here with the same value?

::: media/mtransport/test_nr_socket.cpp:478
(Diff revision 1)
>    if (connect_invoked_ || !port_mappings_.empty()) {
>      MOZ_CRASH("TestNrSocket::connect() called more than once!");
>      return R_INTERNAL;
>    }
>  
> +  if (strlen(addr->tls_host)) {

strlen() appears to me like a waste of time here. How about something like: 
  if (addr->tls_host[0] != '\0') {

::: media/mtransport/test_nr_socket.cpp:520
(Diff revision 1)
> +    r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s dropping outgoing TCP because STUN",
> +          my_addr().as_string);

"... dropping outgoing STUN TCP message because blocking STUN is requested" ?

::: media/mtransport/test_nr_socket.cpp:527
(Diff revision 1)
> +    r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s dropping outgoing TCP because TCP",
> +          my_addr().as_string);

"... dropping outgoing TCP connection because TCP blocking is requested"?

::: media/mtransport/test_nr_socket.cpp:541
(Diff revision 1)
> -      return -1;
> +      r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s dropping outgoing TCP because timeout",
> +            my_addr().as_string);

"... dropping outgoing TCP connection because port mappings timed out"?

::: media/mtransport/test_nr_socket.cpp:579
(Diff revision 1)
> +  UCHAR *cbuf = static_cast<UCHAR*>(const_cast<void*>(buf));
> +  if (nat_->block_stun_ && nr_is_stun_message(cbuf, *len)) {

How can this if() ever be true, with the 'if (r) {' early exit a couple of lines above?
If I'm not mistaken we should get here only if r is zero.
Attachment #8823466 - Flags: review?(drno) → review+

Comment 23

11 months ago
mozreview-review
Comment on attachment 8823467 [details]
Bug 1056934 - Part 3: Make it possible to configure TURN TLS servers in nICEr.

https://reviewboard.mozilla.org/r/101990/#review102776

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:671
(Diff revision 1)
> +      strncpy(cand->stun_server_addr.tls_host,
> +              cand->stun_server->u.dnsname.host,
> +              sizeof(cand->stun_server_addr.tls_host));

Add zero termination at the end of |tls_host| to avoid problems when |u.dnsname.host| is bigger then |tls_host| and string in |tls_host| therefore might not be zero terminated.
Attachment #8823467 - Flags: review?(drno) → review+

Comment 24

11 months ago
mozreview-review
Comment on attachment 8823468 [details]
Bug 1056934 - Part 4: Handle turns URLs when configuring nICEr.

https://reviewboard.mozilla.org/r/101992/#review102780
Attachment #8823468 - Flags: review?(drno) → review+

Comment 25

11 months ago
mozreview-review
Comment on attachment 8823469 [details]
Bug 1056934 - Part 5: Open TLS sockets when communicating with a TLS endpoint.

https://reviewboard.mozilla.org/r/101994/#review102782

::: media/mtransport/nr_socket_prsock.cpp:878
(Diff revision 1)
>    int r,_status;
>    PRNetAddr naddr;
>    int32_t connect_status, getsockname_status;
>  
> +  // TODO: Add TLS layer with nsISocketProviderService?
> +  if (strlen(addr->tls_host))

Again I think strlen() is not the best way to detect if tls_host is set.
Attachment #8823469 - Flags: review?(drno) → review+
(Assignee)

Comment 26

11 months ago
mozreview-review-reply
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> Is there a reason for over writing the constructor default value here with the same value?

I think this is leftover from earlier work. Will remove.

> strlen() appears to me like a waste of time here. How about something like: 
>   if (addr->tls_host[0] != '\0') {

I'm fine with doing it this way too.

> How can this if() ever be true, with the 'if (r) {' early exit a couple of lines above?
> If I'm not mistaken we should get here only if r is zero.

If we get this far in, yes r==0, but I'm not sure how it follows that this will never be true...

Comment 27

11 months ago
mozreview-review
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review103020
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

11 months ago
mozreview-review-reply
Comment on attachment 8823470 [details]
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.

https://reviewboard.mozilla.org/r/101996/#review102572

> do you want starttls here, or (nullptr, 0) ?

I was just doing what code elsewhere does, although I guess we'll never be switching protocol.

Comment 35

11 months ago
mozreview-review-reply
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> If we get this far in, yes r==0, but I'm not sure how it follows that this will never be true...

So r holds the amount of bytes read I assume.
If any bytes got read the "if (r)" in 570 will return.
So from line 573 on r is zero. How can nr_is_stun_message() on an empty buffer (I assume bytes from a previous partial read are not stored in this buffer) ever be true?

Comment 36

11 months ago
mozreview-review
Comment on attachment 8823470 [details]
Bug 1056934 - Part 6: Fix bug where TCPSocketParent would not use TLS when requested.

https://reviewboard.mozilla.org/r/101996/#review103540

I think changing it be null is the conservative and right thing.. r+ with that
Attachment #8823470 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 37

10 months ago
mozreview-review-reply
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> So r holds the amount of bytes read I assume.
> If any bytes got read the "if (r)" in 570 will return.
> So from line 573 on r is zero. How can nr_is_stun_message() on an empty buffer (I assume bytes from a previous partial read are not stored in this buffer) ever be true?

r holds the error code. The previous code was totally wrong. (yikes)

Comment 38

10 months ago
mozreview-review-reply
Comment on attachment 8823466 [details]
Bug 1056934 - Part 2: Test-case for TURN TLS.

https://reviewboard.mozilla.org/r/101988/#review102774

> r holds the error code. The previous code was totally wrong. (yikes)

I should have known better. This make more sense now.
(Assignee)

Comment 39

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e121f409c57d
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 52

10 months ago
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9a5828b9c89
Part 1: TLS support in the test ICE server. r=drno
https://hg.mozilla.org/integration/autoland/rev/341ba823f532
Part 2: Test-case for TURN TLS. r=drno
https://hg.mozilla.org/integration/autoland/rev/0eb5a5b7809b
Part 3: Make it possible to configure TURN TLS servers in nICEr. r=drno
https://hg.mozilla.org/integration/autoland/rev/8689021c7d76
Part 4: Handle turns URLs when configuring nICEr. r=drno
https://hg.mozilla.org/integration/autoland/rev/c100d0ad4d4a
Part 5: Open TLS sockets when communicating with a TLS endpoint. r=drno
https://hg.mozilla.org/integration/autoland/rev/a78433dca4bb
Part 6: Fix bug where TCPSocketParent would not use TLS when requested. r=mcmanus

Comment 53

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a9a5828b9c89
https://hg.mozilla.org/mozilla-central/rev/341ba823f532
https://hg.mozilla.org/mozilla-central/rev/0eb5a5b7809b
https://hg.mozilla.org/mozilla-central/rev/8689021c7d76
https://hg.mozilla.org/mozilla-central/rev/c100d0ad4d4a
https://hg.mozilla.org/mozilla-central/rev/a78433dca4bb
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago10 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 54

10 months ago
Hey,

would it be possible to schedule this release for mozilla52 instead? We made the experience that in Enterprise Environments (FF ESR & MacAffee WebGateway Proxies & Firewall on Symmetric NAT) FireFox behaves very unstable. We suspect that TURN over TLS would help a lot in these Situations (at lease A/B tests with Chrome seem to support this). Almost all of our customers do deploy FF ESR in their ecosystem. If the feature could be scheduled one release earlier, we could see it in the upcoming major ESR Release. Later releases would probably delay the feature for restrictive enterprises for a complete year ....

Thanks a lot
Byron, Maire -- we noticed this feature on the Fx53 tracking board. Is manual testing required for it?
Flags: qe-verify?
Flags: needinfo?(mreavy)
Flags: needinfo?(docfaraday)
(Assignee)

Comment 56

10 months ago
Yes, this will require manual testing (unfortunately). I'll send you mail with a test case.
Flags: needinfo?(docfaraday)
Flags: needinfo?(mreavy)

Comment 57

10 months ago
(In reply to pantherdesign from comment #54)
> would it be possible to schedule this release for mozilla52 instead? We made
> the experience that in Enterprise Environments (FF ESR & MacAffee WebGateway
> Proxies & Firewall on Symmetric NAT) FireFox behaves very unstable. We
> suspect that TURN over TLS would help a lot in these Situations (at lease
> A/B tests with Chrome seem to support this). Almost all of our customers do
> deploy FF ESR in their ecosystem. If the feature could be scheduled one
> release earlier, we could see it in the upcoming major ESR Release. Later
> releases would probably delay the feature for restrictive enterprises for a
> complete year ....

Chromes implementation is different in lots of other ways, so just the fact that Chrome is more successful in connecting is not very convincing to me that this particular feature makes the difference. It could be, but could also be something else.
Could you please take Firefox 53 (or 54) and test if it helps at all and report back?
Flags: needinfo?(pantherdesign)
(In reply to Byron Campen [:bwc] from comment #56)
> Yes, this will require manual testing (unfortunately). I'll send you mail
> with a test case.

Any update on this?
Flags: needinfo?(docfaraday)
(Assignee)

Comment 59

8 months ago
Sent mail just now.
Flags: needinfo?(docfaraday)
Flags: qe-verify? → qe-verify+
QA Contact: alexandru.simonca
I used Firefox Beta 53.0b6 with the pref browser.tabs.remote.autostart set to true (to be sure that the browser has e10s enabled) on Windows 10 x64, Mac OS X 10.12.4 & Ubuntu 16.04 LTS x64. 
This issue is VERIFIED FIXED.
You can check the steps I followed and the results at the following link:
 - https://public.etherpad-mozilla.org/p/TURN-TLS
Status: RESOLVED → VERIFIED
status-firefox53: fixed → verified
Flags: qe-verify+

Comment 61

7 months ago
Guess that this patch introduced an issue: in Firefox version 53 and above create RTCPeerConnection with *turns* scheme and IP address throws exception. From Developer Tools Console:
---
new RTCPeerConnection({iceServers:[{urls:["turns:127.0.0.1:443?transport=tcp"],credential:"ABC",username:"test@example.com:12345"}]});
"[Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/components/PeerConnection.js :: __init :: line 457"  data: no]"
TURNS is not yet supported.  PeerConnection.js:719
---

*turns* scheme works with hostname:
---
new RTCPeerConnection({iceServers:[{urls:["turns:localhost:443?transport=tcp"],credential:"ABC",username:"test@example.com:12345"}]});
RTCPeerConnection { _innerObject: Object, localDescription: null, remoteDescription: null, signalingState: "stable", canTrickleIceCandidates: null, iceGatheringState: "new", iceConnectionState: "new", peerIdentity: Promise, idpLoginUrl: null, onnegotiationneeded: null }
TURNS is not yet supported.  PeerConnection.js:719
---

and *turn* scheme works with IP address and hostname as well:
---
new RTCPeerConnection({iceServers:[{urls:["turn:127.0.0.1:443?transport=tcp"],credential:"ABC",username:"test@example.com:12345"}]});
RTCPeerConnection { _innerObject: Object, localDescription: null, remoteDescription: null, signalingState: "stable", canTrickleIceCandidates: null, iceGatheringState: "new", iceConnectionState: "new", peerIdentity: Promise, idpLoginUrl: null, onnegotiationneeded: null }
---

The issue appears to be only if *turns* scheme is combined with IP address.

Comment 62

7 months ago
Got following logs from the latest nightly - 55.0a1 (2017-04-13) (64-bit):

---
2017-04-13 14:42:41.043947 UTC - [Main Thread]: I/signaling [main|PeerConnectionImpl] PeerConnectionImpl.cpp:331: PeerConnectionImpl: PeerConnectionImpl constructor for 
2017-04-13 14:42:41.043972 UTC - [Main Thread]: D/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:184: Created PeerConnection: 0x7fc306ed8260
2017-04-13 14:42:41.046490 UTC - [Main Thread]: D/signaling [main|PeerConnectionCtx] PeerConnectionCtx.cpp:142: Creating PeerConnectionCtx
2017-04-13 14:42:41.046897 UTC - [Main Thread]: D/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:296: 13e89bbe6b9ca257: Get stun addresses via IPC
2017-04-13 14:42:41.047035 UTC - [Socket Thread]: D/mtransport NrIceCtx static call to find local stun addresses

2017-04-13 14:42:41.047243 UTC - [Main Thread]: E/mtransport Couldn't set TURN server for 'PC:1492094561046446 (id=6442450947 url=about:home)'
2017-04-13 14:42:41.047259 UTC - [Main Thread]: E/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:414: Init: Failed to set turn servers
2017-04-13 14:42:41.047265 UTC - [Main Thread]: E/signaling [main|PeerConnectionImpl] PeerConnectionImpl.cpp:676: Initialize: Couldn't initialize media object

2017-04-13 14:42:41.058659 UTC - [Main Thread]: I/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:209: OnProxyAvailable: Proxy Available: 0
2017-04-13 14:42:41.058672 UTC - [Main Thread]: I/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:226: SetProxyOnPcm: Had proxyinfo
2017-04-13 14:42:41.058717 UTC - [Main Thread]: I/signaling [main|PeerConnectionMedia] PeerConnectionMedia.cpp:267: OnStunAddrsAvailable: receiving (5) stun addrs
---

hope this helps. Do you want me to create a new bug for this?
turns with an ip address is very very unlikely to work unless you have a signed certificate for an ip address...

Comment 64

7 months ago
That's right. Its a rare case, but still valid. And even if it's not implemented Firefox should not throw an exception, should it?
Before version 53 there was just a warning that TURNS is not supported. Now TURNS is supported, but Firefox throws an exception for URL with IP address. I think it will be more consistent to keep the warning instead.
Moreover in my case there are 3 URLs in configuration - for all transports UDP, TCP and TLS:
---
{iceServers:[{
  urls:[
    "turn:127.0.0.1:3478?transport=udp",
    "turn:127.0.0.1:443?transport=tcp",
    "turns:127.0.0.1:443?transport=tcp"],
  credential:"ABC",
  username:"test@example.com:12345"
}]}
---
and because the last one is not supported Firefox throws exception and cannot connect.

My point is, it's great that TURNS is implemented now. I really like it, but it should not break applications that used to work without TURNS support prior version 53.
(Assignee)

Comment 65

7 months ago
With any new feature comes new potential error conditions related to that feature, and using an IP address with turns is just broken.

Comment 66

7 months ago
Not exactly. TURNS didn't worked in previous version, so it cannot be broken.
What is broken (in example above) is TURN UDP and TCP with IP address. TURNS with IP address is just not allowed, but brakes the other "acceptable" TURN transports, which worked with version 52.

Your statement sounds like you agree with me that this is a bug. Right? So since this bug is closed I guess the best option is to create a new one. What do you think?
(Assignee)

Comment 67

7 months ago
Trying to use TLS with an IP address is terrible enough that it warrants a full stop, IMHO. You need to stop doing this.

Comment 68

4 months ago
When I go to https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/
and fill in a TURNS server, say "turns:turns.fig.org:443?transport=tcp" with authentication, everything looks good on the candidates report (there is a relay candidate with the server's IP), but on the Javascript debug console, I get:

TURNS is not yet supported.  main.js:119

This is with Firefox 54.  I'm not sure why the error comes up on the Trickle ICE page, as the WebRTC application I am using is definitely doing relaying over TLS with the same ICE configuration, with no errors.

Thanks.

Updated

4 months ago
Blocks: 1383575
(In reply to michael from comment #68)
> When I go to
> https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/
> and fill in a TURNS server, say "turns:turns.fig.org:443?transport=tcp" with
> authentication, everything looks good on the candidates report (there is a
> relay candidate with the server's IP), but on the Javascript debug console,
> I get:
> 
> TURNS is not yet supported.  main.js:119
> 
> This is with Firefox 54.  I'm not sure why the error comes up on the Trickle
> ICE page, as the WebRTC application I am using is definitely doing relaying
> over TLS with the same ICE configuration, with no errors.
> 
> Thanks.

The JS console log warning is a left over from this bug. I opened bug 1383575 for you to remove the warning.
(In reply to michael from comment #68)
> When I go to
> https://webrtc.github.io/samples/src/content/peerconnection/trickle-ice/
> and fill in a TURNS server, say "turns:turns.fig.org:443?transport=tcp" with
> authentication, everything looks good on the candidates report (there is a
> relay candidate with the server's IP), but on the Javascript debug console,
> I get:
> 
> TURNS is not yet supported.  main.js:119
> 
> This is with Firefox 54.  I'm not sure why the error comes up on the Trickle
> ICE page, as the WebRTC application I am using is definitely doing relaying
> over TLS with the same ICE configuration, with no errors.

The reason it is not working is that turns.fig.org doesn't even establish a TCP connection on port 443.
You need to log in before you can comment on or make changes to this bug.