Closed Bug 1264110 Opened 4 years ago Closed 3 years ago

Intermittent test_presentation_tcp_sender_establish_connection_error.html | Test timed out.

Categories

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

48 Branch
defect

Tracking

()

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

People

(Reporter: KWierso, Assigned: schien)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

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: 3 years ago
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+
Duplicate of this bug: 1293009
Duplicate of this bug: 1293011
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.