Closed Bug 1264110 Opened 9 years ago Closed 9 years ago

Intermittent test_presentation_tcp_sender_establish_connection_error.html | Test timed out.

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
48 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox48 --- wontfix
firefox49 --- fix-optional
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: KWierso, Assigned: schien)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Bug 1264110 - fix timing issue in test cases. .
58 bytes, text/x-review-board-request
kershaw
: review+
Details
aurora-uplift.patch
32.35 KB, patch
ritu
: approval-mozilla-aurora+
Details | Diff | Splinter Review
This intermittent failure is reported after we land the data channel integration. I'd like to double check the TCP session transport builder code to see if anything fishy.
I'll take a look at this intermittent failure.
Assignee: nobody → schien
Flags: needinfo?(schien)
Attachment #8786569 - Flags: review?(schien)
Comment on attachment 8786569 [details] Bug 1264110 - fix timing issue in test cases. . https://reviewboard.mozilla.org/r/75478/#review73440
Attachment #8786569 - Flags: review?(schien) → review+
Turn on corresponding NSPR log to get more information.
Keywords: leave-open
Pushed by schien@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ece74cf8b9f5 turn on Presentation logging on treeherder. r=me.
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Timing issue confirmed, this test case needs to be fixed. The |NotifyTransportClosed| is not actually fired on the presentation session we expected. The test case is passed when GC kicks in. >09:20:09 INFO - [Main Thread]: D/Presentation StartSession:url[http://example.com/], id[{e5267801-501e-6946-9858-94167950188c}] >09:20:09 INFO - [Main Thread]: D/Presentation Init:ServerSocket created.port[5678] >09:20:09 INFO - [Main Thread]: D/Presentation RegisterSessionListener:id[{e5267801-501e-6946-9858-94167950188c}], role[1] >09:20:09 INFO - [Main Thread]: D/Presentation connection state change:id[{e5267801-501e-6946-9858-94167950188c}], state[0], reason[0], role[1] >09:20:09 INFO - [Main Thread]: D/Presentation NotifyConnected:id[{e5267801-501e-6946-9858-94167950188c}], role[1] >09:20:09 INFO - [Main Thread]: D/Presentation OnSocketAccepted:receive from port[0] >09:20:09 INFO - [Main Thread]: D/Presentation NotifyTransportClosed:id[{88f6a26c-315e-544b-bfc7-8f0debe89ba4}], reason[8000ffff], role[1] >09:20:09 INFO - [Main Thread]: D/Presentation Shutdown:id[{88f6a26c-315e-544b-bfc7-8f0debe89ba4}], reason[8000ffff], role[1] >09:20:09 INFO - [Main Thread]: D/Presentation OnSessionTransport:id[{e5267801-501e-6946-9858-94167950188c}], role[1] >09:20:09 INFO - [Main Thread]: D/Presentation NotifyTransportReady:id[{e5267801-501e-6946-9858-94167950188c}], role[1]
Comment on attachment 8786569 [details] Bug 1264110 - fix timing issue in test cases. . https://reviewboard.mozilla.org/r/75478/#review75000 ::: dom/presentation/PresentationSessionInfo.cpp:500 (Diff revision 2) > SetBuilder(nullptr); > > + if (mState != nsIPresentationSessionListener::STATE_CONNECTING) { > + return NS_ERROR_FAILURE; > + } > + Would it be better to move this check before |Setbuilder(nullptr)|? ::: dom/presentation/PresentationSessionInfo.cpp:1363 (Diff revision 2) > +PresentationPresentingInfo::DoReconnect() > +{ > + PRES_DEBUG("%s:id[%s], role[%d]\n", __func__, > + NS_ConvertUTF16toUTF8(mSessionId).get(), mRole); > + > + MOZ_ASSERT(mState == nsIPresentationSessionListener::STATE_CLOSED); At controller side, it is possible to reconnect to a connected connection. For that case, we might need to close the connection in |PresentationControllingInfo::Reconnect|. Maybe add something like: if (mState == nsIPresentationSessionListener::STATE_CONNECTED)) { Close(NS_OK, nsIPresentationSessionListener::STATE_CLOSED); }
Comment on attachment 8786569 [details] Bug 1264110 - fix timing issue in test cases. . https://reviewboard.mozilla.org/r/75478/#review75712 ::: dom/presentation/PresentationSessionInfo.cpp:500 (Diff revision 2) > SetBuilder(nullptr); > > + if (mState != nsIPresentationSessionListener::STATE_CONNECTING) { > + return NS_ERROR_FAILURE; > + } > + Probably not, need to break the reference loop between session info and builder for both success and fail path.
Attachment #8786569 - Flags: review+
Comment on attachment 8786569 [details] Bug 1264110 - fix timing issue in test cases. . https://reviewboard.mozilla.org/r/75478/#review75722
Attachment #8786569 - Flags: review?(kechang) → review+
Pushed by cbook@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/817e76d33b76 fix timing issue in test cases. r=kershaw.
https://hg.mozilla.org/mozilla-central/rev/817e76d33b76
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
If this affects other channels maybe it makes sense to uplift the test fixes, at least to aurora. What do you think? Or, does it only affect nightly tests?
Flags: needinfo?(schien)
That would be nice. Note that it'll need rebasing as well.
Let's see how this works on try.
Flags: needinfo?(schien)
Comment on attachment 8790183 [details] [diff] [review] aurora-uplift.patch Approval Request Comment [Feature/regressing bug #]:bug 1264513 [User impact if declined]: n/a [Describe test coverage new/current, TreeHerder]: mochitest [Risks and why]: low, fix for test automation. [String/UUID change made/needed]: n/a
Attachment #8790183 - Flags: approval-mozilla-aurora?
Comment on attachment 8790183 [details] [diff] [review] aurora-uplift.patch Fixes an intermittent test failure, Aurora50+
Attachment #8790183 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: