Random orange: TEST-UNEXPECTED-FAIL | test_videoAllowedInMail

RESOLVED FIXED in Thunderbird 3.1a1

Status

defect
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({intermittent-failure})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird3.0 .2-fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

10 years ago
TEST-UNEXPECTED-FAIL |  test_videoAllowedInMail
  EXCEPTION: Video has not been blocked in message content as expected.
    at: test-video-content-policy.js line 205
       Error("Video has not been blocked in message content as expected.")  0
       test_videoAllowedInMail() test-video-content-policy.js 205
            frame.js 460
            frame.js 512
            frame.js 554
            frame.js 403
            frame.js 559
            server.js 164
            server.js 168

http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTest/1263533744.1263536301.8530.gz
(Assignee)

Comment 1

10 years ago
Looking at the test, I obviously did some incorrect copy and paste on the failure statements. This patch corrects those.

For the failure case, this bug adds a delay and then some extra debugging. It does not make the test pass. My suspicion is that wait_for_message_display_completion isn't doing what it should be, but I want to catch it in the act.
Attachment #421806 - Flags: review?(sid.bugzilla)
Attachment #421806 - Flags: review?(sid.bugzilla) → review+
You might want to try passing 'true' for wait_for_message_display_completion's aLoadDemanded argument.  By default, the method only does something if a load is in progress.  If the load is not reliably initiated, the method will end up not actually doing any waiting...

Code with the doc block:
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-folder-display-helpers.js#1060
(Assignee)

Comment 3

10 years ago
(In reply to comment #2)
> You might want to try passing 'true' for wait_for_message_display_completion's
> aLoadDemanded argument.  By default, the method only does something if a load
> is in progress.  If the load is not reliably initiated, the method will end up
> not actually doing any waiting...

Whilst I believe the load should always be initiated (we end up calling reloadMessage on the dbView) there could be something I'm not aware of.

This failure is occurring a couple of times a day, so I've checked in the patch as-is to confirm that the allowing of remote content is happening, and there isn't some weirder kind of error. Once we get the debug, then I'll probably try adding the true parameter.
(Assignee)

Comment 4

10 years ago
Comment on attachment 421806 [details] [diff] [review]
[checked in] Fix comments, add debug.

http://hg.mozilla.org/comm-central/rev/180c22a0e1b1
Attachment #421806 - Attachment description: Fix comments, add debug. → [checked in] Fix comments, add debug.
(Assignee)

Comment 5

10 years ago
The error occurred again very quickly and we got:

Video was unexpectedly blocked. Network State was: 4After a 3 second sleep, the network state is now: 1

Which means after a bit longer it did work fine.
(Assignee)

Comment 6

10 years ago
Posted patch Proposed fixSplinter Review
Based on asuth's comments - pass true to wait_for_message_display_completion and remove the debug code. Sid tells me that onclick isn't guaranteed to be synchronous and this follows what is happening in the test.
Attachment #422094 - Flags: review?(sid.bugzilla)
Attachment #422094 - Flags: review?(sid.bugzilla) → review+
Comment on attachment 422094 [details] [diff] [review]
Proposed fix

Nit: You could get rid of the braces around the now single-statement if.

Let's see if this fixes things.
I'm wondering if we're missing the case where we've returned without yet firing the oncommand handler at <http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1872>, and so messageLoaded hasn't yet become false. This would mean that we pass the isLoadedChecker check without waiting for messageLoaded to become false, then true. asuth, could something like this happen?

One solution to this might be to call LoadMsgWithRemoteContent() directly.
s/to become false, then true/to become true/
(In reply to comment #9)
> I'm wondering if we're missing the case where we've returned without yet firing
> the oncommand handler at

This sounds very possible.  When I read all of the docs for wait_for_message_display_completion, I notice that it requires messageLoaded to be false at that point when requiring a load.  I think it's slightly incorrect about that; what we want to require is that messageLoaded was forced or verified to be false prior to triggering the action that would initiate a load.  (Otherwise wait_for_message_display_completion could be a more friendly API and just clobber the variable itself.)

It's probably best to abstract this issue by introducing a new helper "plan_for_message_display" that clobbers messageLoaded but could do something more elegant in the future.

> One solution to this might be to call LoadMsgWithRemoteContent() directly.

This seems less desirable than the above.

I've implemented the former fix and verified this solves the problem locally.  To enable the verification I've added a simple augmented helper function to the controller called defer_click which allows us to synthesize the presumed failure in this case.  Without the call to 'plan_for_message_display' and using defer_click the test appears to fail per this bug's description.
Attachment #422157 - Flags: review?(bugzilla)
The patch looks good.

I think having them both set to false instead of null initially would make more sense -- also, we should probably do this with other wait_for_message_display_completion(_, true) calls too.
Comment on attachment 422157 [details] [diff] [review]
add plan_for_message_display, debug helper, and improve docs v1

treating previous comment as r=sid0.  I'll make the null/false change and push.  (At some point I adopted null as a marker that an attribute exists on the prototype but should be initialized in the constructor; but since we aren't initializing in the constructor here, that makes no sense.)

Leaving the other wait_for_message_display_completion stuff for another patch for now, noting that I change all of the cases in folder-display already.  (The cases not changed were for new windows where it's not required, as per my docs.)
Attachment #422157 - Flags: review?(bugzilla) → review+
We've had around 50 test runs since, and the test hasn't failed even once. Looks like it's fixed.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
OS: Windows 2000 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.1a1
Attachment #422157 - Flags: approval-thunderbird3.0.2?
Comment on attachment 422157 [details] [diff] [review]
add plan_for_message_display, debug helper, and improve docs v1

NPOTB, test-only correctness fix.
(Assignee)

Comment 17

10 years ago
Comment on attachment 422157 [details] [diff] [review]
add plan_for_message_display, debug helper, and improve docs v1

a=Standard8 but you should land the other two patches as well (especially the first to get the test failure text right).
Attachment #422157 - Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2+
(Assignee)

Updated

10 years ago
Attachment #421806 - Flags: approval-thunderbird3.0.2+
(Assignee)

Updated

10 years ago
Attachment #422094 - Flags: approval-thunderbird3.0.2+
(Assignee)

Comment 19

9 years ago
Verifying as fixed for 3.0.2 based on the fact that these are test-only patches and the last 5 runs from MozMill/unit tests have been green with no failures relating to these patches.
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.