Closed Bug 1052775 Opened 6 years ago Closed 6 years ago

test_call_start_from_end_handler.html is logging results after SimpleTest.finish()

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: martijn.martijn, Assigned: ggp)

References

()

Details

Attachments

(1 file, 4 obsolete files)

This is a spin-off from bug 1032878. In that bug, I basically try to fix all tests that were still logging subresults after SimpleTest.finish() was called.

I tried to fix this test in that bug, but my fix caused leaks. This is the patch that seemed to cause leaks: https://bug1032878.bugzilla.mozilla.org/attachment.cgi?id=8470599
I don't think that should cause leaks, should it?
Attached patch 1052775.diff (obsolete) — Splinter Review
This is a patch that seems to fix this issue and not cause any leaks.
But this setTimeout call is really weird.
Guilherme, could you help me fix this issue. Perhaps, there is another way to fix this.
I guess, ideally, the leak issue should be fixed.
Blocks: 1032878
Flags: needinfo?(ggoncalves)
Attached patch WIP (obsolete) — Splinter Review
I believe you have exposed some deeper flaws in SpeechRecognition; sorry about that.
I'll have to take a deeper look later, but meanwhile, can you check whether this patch
fixes your initial problem, and that it doesn't leak?
Flags: needinfo?(ggoncalves)
Attached patch 1052775.diff (obsolete) — Splinter Review
Yes, that seems to fix the problem and doesn't leak, thanks!
Filed bug 1055093 for what I believe to be the underlying issue here. I'll have a patch shortly.
Attachment #8471830 - Attachment is obsolete: true
Attachment #8471932 - Attachment is obsolete: true
Attachment #8471993 - Attachment is obsolete: true
Attachment #8474639 - Attachment is obsolete: true
Attachment #8474639 - Flags: review?(bugs)
Attachment #8474640 - Flags: review?(bugs)
Attachment #8474640 - Flags: review?(bugs) → review+
Try is looking good: https://tbpl.mozilla.org/?tree=Try&rev=5f6250ca8856

However, I don't see any of our logging messages at all in the logs. Martjin, is this expected? Just want to make sure the problem is fixed, and if it is, we should be clear to land :)
Flags: needinfo?(martijn.martijn)
Keywords: checkin-needed
Sorry, no checkin-needed for now.
Keywords: checkin-needed
(In reply to Guilherme Gonçalves [:ggp] from comment #8)
> Try is looking good: https://tbpl.mozilla.org/?tree=Try&rev=5f6250ca8856
> 
> However, I don't see any of our logging messages at all in the logs.
> Martjin, is this expected? Just want to make sure the problem is fixed, and
> if it is, we should be clear to land :)

This had to be tested in combination with the patch: https://bug1032878.bugzilla.mozilla.org/attachment.cgi?id=8467931 (patch  1032878_v4.diff from bug 1032878) to make sure that it didn't cause failures.
I just did that and it seems fine and I don't see any leaks with it.
Thanks for your help!
Flags: needinfo?(martijn.martijn)
Keywords: checkin-needed
Assignee: nobody → ggoncalves
https://hg.mozilla.org/mozilla-central/rev/1e3273854ca7
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.