Opened 4 years ago

Closed 4 years ago

#9069 closed defect (fixed)

Set have_websocket_binary_frames for Firefox

Reported by: dcf Owned by: dcf
Priority: Medium Milestone:
Component: Archived/Flashproxy Version:
Severity: Keywords: easy
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Firefox 10 for sure doesn't support binary frames. Some newer version probably does. Find out what version and put it in have_websocket_binary_frames.

Child Tickets

Attachments (4)

0001-Set-have_websocket_binary_frames-for-Firefox.patch (3.6 KB) - added by arlolra 4 years ago.
9069.patch (5.0 KB) - added by arlolra 4 years ago.
0001-Make-null-comparison-explicit.patch (934 bytes) - added by arlolra 4 years ago.
0001-Test-if-userAgent-is-a-string.patch (3.2 KB) - added by arlolra 4 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 4 years ago by arlolra

Status: newneeds_review

Firefox 11 sure does.
https://developer.mozilla.org/en-US/docs/WebSockets

Added a patch and tested on 10 / 11 to verify that it works on both.

comment:2 in reply to:  1 Changed 4 years ago by dcf

Status: needs_reviewneeds_revision

Replying to arlolra:

Firefox 11 sure does.
https://developer.mozilla.org/en-US/docs/WebSockets

Added a patch and tested on 10 / 11 to verify that it works on both.

Could you split this into three separate patches?

  • One to refactor have_websocket_binary_frames,
  • One to add tests for have_websocket_binary_frames,
  • One to add Firefox 11 to have_websocket_binary_frames.

What I do is generate three patches with git format-patch HEAD^^^ and then cat them together so they can be applied with git am.

Changed 4 years ago by arlolra

Attachment: 9069.patch added

comment:3 Changed 4 years ago by arlolra

Status: needs_revisionneeds_review

Please see the attached.

comment:4 in reply to:  3 ; Changed 4 years ago by dcf

Status: needs_reviewneeds_information

Replying to arlolra:

Please see the attached.

Can you explain this diff?

-    if (ua === null)
+    if (ua == null)

If ua can be other than null but some other falsy value (what value?), I think I prefer to write it as

-    if (ua === null)
+    if (!ua)

comment:5 in reply to:  4 Changed 4 years ago by arlolra

Replying to dcf:

Can you explain this diff?

-    if (ua === null)
+    if (ua == null)

Double equals null is shorthand for,

 if ( ua === null || ua === undefined )

See the spec,
http://ecma-international.org/ecma-262/5.1/#sec-11.9.3

I'll make that explicit so it's clearer.

Changed 4 years ago by arlolra

comment:6 Changed 4 years ago by arlolra

Apply this one after the other three.

comment:7 Changed 4 years ago by dcf

Replying to arlolra:

Replying to dcf:

Can you explain this diff?

-    if (ua === null)
+    if (ua == null)

Double equals null is shorthand for,

 if ( ua === null || ua === undefined )

Right, I understand that. But I try to avoid semi-mysterious behavior like that. The flashproxy source code doesn't use double-equals anywhere for that reason. I think after I read that Douglas Crockford recommended never to use double-equals, and it made sense to me, so I started doing it.

The change seems unrelated to the rest of the patch. Is it a bug that we are not currently checking against undefined? I mean, what caused you to make the change, were your tests failing or something? If so, we should fix and document it separately. If we have to fix it here, there are probably other places we have to fix it.

Last edited 4 years ago by dcf (previous) (diff)

comment:8 in reply to:  7 Changed 4 years ago by arlolra

Replying to dcf:

The change seems unrelated to the rest of the patch. Is it a bug that we are not currently checking against undefined? I mean, what caused you to make the change, were your tests failing or something? If so, we should fix and document it separately. If we have to fix it here, there are probably other places we have to fix it.

The reason it came up is because in the test I added a case,

{ expected: false },  // no userAgent 

which sets ua to undefined.

This is a problem because later ua is assumed to be a string,

ua.match( ... )

Maybe the best thing would be to do,

typeof ua !== "string"

There looks to another problem in flashproxy_should_disable(). If ua is null there, it skips the first if block and blows up. I can supply a patch for these if you want.

Changed 4 years ago by arlolra

comment:9 Changed 4 years ago by arlolra

Attached a patch that tests for strings and fixes up the code path in flashproxy_should_disable().

comment:10 Changed 4 years ago by dcf

Resolution: fixed
Status: needs_informationclosed

Thanks for your patch. I really think !ua suffices, so I did that, and then merged your changes on top of it. Thank you particularly for the new tests.

Note: See TracTickets for help on using tickets.