1313385 - WebSocketImpl::WebSocketImpl does not initialise mSecure

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Julian Seward [:jseward]]

This is dynamically observable in our test suites.  STR to follow.

Setting as sec-sensitive because it seems to me to be ungood to be
confused about whether a connection is secure or not.  Please declassify
as appropriate.

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaint

Comment 2

[Julian Seward [:jseward]]

Looking at dom/base/WebSocket.cpp, it seems that class WebSocketImpl
has a |bool mSecure|, but |explicit WebSocketImpl(WebSocket* aWebSocket)|
does not give an initial value for it.  I am hypothesising that this is
the cause of the reported complaint.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: ws.patch

Comment 4

[Julian Seward [:jseward]]

(In reply to Andrea Marchesini [:baku] from comment #3)
> Created attachment 8805213 [details] [diff] [review]
> ws.patch

Verified as fixing the complaint.

The test is devtools/server/tests/mochitest/test_websocket-server.html

FTR, command line I used is

DISPLAY=:1.0 G_SLICE=always-malloc ./mach mochitest -f chrome --debugger=v312BRANCH \
  --debugger-args="--fair-sched=yes --smc-check=all-non-file \
  --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc-may2015.supp --error-limit=no \
  --trace-children=yes \
  --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java \
  --num-transtab-sectors=24 --tool=memcheck --freelist-vol=300000000 --redzone-size=64 \
  --gen-suppressions=no --px-default=allregs-at-mem-access \
  --px-file-backed=unwindregs-at-mem-access --stats=no --show-mismatched-frees=no \
  --fullpath-after=/MOZ/ --num-callers=20 --track-origins=no" --keep-open=no \
  devtools/server/tests/mochitest/test_websocket-server.html 2>&1 | tee spew-01-06-mc

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b734eaa69060

Comment 6

[Marcia Knous [:marcia]]

Tracking 52- since this is now resolved fixed.

Comment 7

[Gerry Chang [:gchang]]

Hi :dveditz, 
Since 51 is also affected, is this worth uplifting to 51?

Comment 8

[Gerry Chang [:gchang]]

Track 51+ as sec-moderate.

Comment 9

[Daniel Veditz [:dveditz]]

sure, this is a trivially safe fix to uplift to 51 (and ESR for that matter).

Comment 10

[Gerry Chang [:gchang]]

Hi :baku,
could you please nominate this uplift to Aurora51 & ESR?

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8805213 [details] [diff] [review]
ws.patch

Approval Request Comment
[Feature/regressing bug #]: WebSocket ported to webIDL
[User impact if declined]: mSecure is not initialized in the CTOR. In theory until the initialization is completed in other methods, this value is unset.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
[String/UUID change made/needed]: none

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8805213 [details] [diff] [review]
ws.patch

Fix a sec-moderate. Take it in 51 aurora.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0caed9e822ed

Comment 14

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

This may as well wait for the ESR that will go out with 51 (45.7.0).