Closed Bug 805330 Opened 12 years ago Closed 12 years ago

Intermittent test_bug751809.html | Waited too long for plugin to receive the mouse click, | Plugin should have received 1 mouse up event. - got 0, expected 1

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g18 fixed, b2g18-v1.0.1 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
b2g18 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: philor, Assigned: gfritzsche)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 5 obsolete files)

Clean up test_bug751809.html
4.20 KB, patch
gfritzsche
: checkin+
Details | Diff | Splinter Review
Wait for paint flushs, v4
13.44 KB, patch
rcampbell
: review+
Details | Diff | Splinter Review 
+++ This bug was initially created as a clone of Bug #783723 +++

https://tbpl.mozilla.org/php/getParsedLog.php?id=16441157&tree=Mozilla-Inbound
Rev4 MacOSX Snow Leopard 10.6 mozilla-inbound debug test mochitests-3/5 on 2012-10-24 20:08:26 PDT for push 888ac1c46830
slave: talos-r4-snow-043

72 INFO TEST-PASS | /tests/dom/plugins/test/test_bug751809.html | Plugin should not have received mouse events yet.
--DOMWINDOW == 13 (0x1002410d0) [serial = 20] [outer = 0x0] [url = http://mochi.test:8888/tests/dom/plugins/test/test_GCrace.html]
--DOMWINDOW == 12 (0x12a4583e0) [serial = 28] [outer = 0x1277b17d0] [url = http://mochi.test:8888/tests/dom/plugins/test/test_bug539565-2.html]
73 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/test_bug751809.html | Waited too long for plugin to receive the mouse click
74 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/test_bug751809.html | Plugin should have received 1 mouse up event. - got 0, expected 1
75 INFO TEST-END | /tests/dom/plugins/test/test_bug751809.html | finished in 3603ms
Attached patch patch (obsolete) — — Splinter Review 
We've had troubles with this test before, but the (similar) ones in browser/base/content/test/browser_pluginnotification.js seem to work fine. Maybe if we move this there it will work better?
Attachment #675203 - Flags: feedback?(georg.fritzsche)
Comment on attachment 675203 [details] [diff] [review]
patch

Can you make a new test file for this? browser_pluginnotification.js is getting too large. Other than that, I'm fine with this move.
Attachment #675203 - Flags: feedback?(jaws) → feedback+
(In reply to David Keeler from comment #2)
> We've had troubles with this test before, but the (similar) ones in
> browser/base/content/test/browser_pluginnotification.js seem to work fine.
> Maybe if we move this there it will work better?

I assume that the failures are due to the something like the window state being off as waitForFocus() apparently made it much rarer. If the different test setup there solves that, that would be great.
Maybe i overlooked it though, but those don't test for click-reception on the plugin so far, only the C2P overlay?
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> Maybe i overlooked it though, but those don't test for click-reception on
> the plugin so far, only the C2P overlay?

... I of course meant the already existing/similar tests.
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> > Maybe i overlooked it though, but those don't test for click-reception on
> > the plugin so far, only the C2P overlay?
> 
> ... I of course meant the already existing/similar tests.

Correct - they just test whether or not the plugin activates.
This orange seems to be pretty rare, though, so unless someone feels really strongly or unless this starts to fail more often, I'm going to wait on going forward with moving the test.
Alternatively i thought of maybe cleaning this test up to not dynamically insert the plugin etc. in case this leads to timing issues in the test-environment:
https://hg.mozilla.org/try/rev/2b333ca46de9
https://tbpl.mozilla.org/?tree=Try&rev=b4fd38c0a9d4
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Alternatively i thought of maybe cleaning this test up to not dynamically
> insert the plugin etc. in case this leads to timing issues in the
> test-environment:
> https://hg.mozilla.org/try/rev/2b333ca46de9
> https://tbpl.mozilla.org/?tree=Try&rev=b4fd38c0a9d4

That sounds like a better plan, actually. If this fixes it, then we're good. If not, we can always try moving the test later.
Attached patch Clean up test_bug751809.html (obsolete) — — Splinter Review 
Ok, going with the cleanup of this test.

Josh, can you do the review of this follow-up to bug 751809 too?
Assignee: nobody → georg.fritzsche
Status: NEW → ASSIGNED
Attachment #676219 - Flags: review?(joshmoz)
Attachment #675203 - Flags: feedback?(georg.fritzsche)
Comment on attachment 676219 [details] [diff] [review]
Clean up test_bug751809.html

Review of attachment 676219 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine, but I'm curious as to why it was written with document.write in the first place. Hope doing this doesn't interfering with testing what we mean to test, but it doesn't seem like it would.
Attachment #676219 - Flags: review?(joshmoz) → review+
(In reply to Josh Aas (Mozilla Corporation) from comment #19)
> Seems fine, but I'm curious as to why it was written with document.write in
> the first place. Hope doing this doesn't interfering with testing what we
> mean to test, but it doesn't seem like it would.

The document.write() was just me not knowing a better way at the time to set the click_to_play pref properly for the test.

        Attachment #676219 -
        Attachment is obsolete: true

    
  
Keywords: checkin-needed

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2e0eb299471c
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
      
  

    
    

    
  
Alright, so that wasn't it :/
I wonder why this is mac debug only and appeared rather recently.

David, do you want to give the moving a try? I currently can't think of another fix.

    
    

    
  


  
While checking something else i found out that i can actually reproduce this intermittently with a slower debug build.

When delaying the mouse click a bit it doesn't occur anymore, so it looks like plugin.activated (what we use as the check for instantiation here) is true before the plugin actually is properly/fully instantiated.

    
    

    
  
(In reply to Georg Fritzsche [:gfritzsche] from comment #27)
> so it looks like plugin.activated (what we use as the check for 
> instantiation here) is true before the plugin actually is
> properly/fully instantiated.

... which might explain the sudden appearance of this intermittent failure: Bug 797043 (cleaning up nsObjectLoadingContent::mActivated) landed on 10-20.

    
    

    
  
I think the meaning of mActivated has been overloaded in a way that doesn't really work. To part of the code, it means whether or not the user has decided to activate the plugin (ala click-to-play). In other parts of the code (particularly these tests), it seems we want it to mean whether or not the plugin has successfully and fully instantiated. Maybe it would be nice to have an attribute we can access that means this latter thing, but in the meantime, we can get pretty close to that by seeing if a function we're expecting to be exposed by the plugin is available on the plugin object (you might have to use plugin.wrappedJSObject.function or something - I don't quite remember).

    
    

    
  
(In reply to David Keeler from comment #29)
> I think the meaning of mActivated has been overloaded in a way that doesn't
> really work. To part of the code, it means whether or not the user has
> decided to activate the plugin (ala click-to-play). In other parts of the
> code (particularly these tests), it seems we want it to mean whether or not
> the plugin has successfully and fully instantiated. Maybe it would be nice
> to have an attribute we can access that means this latter thing, but in the
> meantime, we can get pretty close to that by seeing if a function we're
> expecting to be exposed by the plugin is available on the plugin object (you
> might have to use plugin.wrappedJSObject.function or something - I don't
> quite remember).

After Bug 797043, activated only means the object passed CTP checks - the plugin instantiation wont happen until either a return to the event loop (channel-less) or the channel reaches OnStartRequest.

If working with the test plugin it's easy enough to check if the plugin has spawned by seeing if any of its functions are defined:
> if (obj.getObjectValue !== undefined)

To differentiate between two instances of running plugins (to see if it respawned)

> let oldPlugin = obj.getObjectValue();
> 
> // ...
> // Do something that should respawn the plugin
> // ...
> 
> let respawned = false;
> try {
>   respawned = obj.checkObjectValue(oldPlugin);
> } catch (e) {}

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Right, thanks. 
Fixing the activation check this runs into another issue:

Mouse clicks aren't delivered if the click occurs before the plugin instance owner sees the plugin as visible:
http://dxr.mozilla.org/mozilla-central/dom/plugins/base/nsPluginInstanceOwner.cpp#l1938

While we can check plugin.IsVisible():
http://dxr.mozilla.org/mozilla-central/dom/plugins/test/testplugin/nptest.cpp.html#l3541
... but before nsPluginInstanceOwner::mWidgetVisible initially becomes true this is hit:
http://dxr.mozilla.org/mozilla-central/dom/plugins/ipc/PluginInstanceChild.cpp.html#l3476
... which sets the plugins clip rect to non-zero dimensions. 

Only quickly skimmed through bug 626602, but i wonder what we need that early temporary visibility patching for. Chris, do you remember this?

    
  

        Attachment #681541 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Flags: needinfo?(jones.chris.g)

    
    

    
  
Yes, roc added this because some plugins break if they're always invisible but aren't painted at least once.
Flags: needinfo?(jones.chris.g)

    
    

    
      
  

    
    

    
  
Alright, if that workaround is required we can either
(1) check wether the browser thinks the plugin is visible or
(2) work around this with a delay to avoid sending the click to the plugin too early

I don't see a way for (1) from the test-environment (only native paths); would it make sense to expose a nsIObjectLoadingContent::isPluginVisible attribute?

    
    

    
      
  

    
    

    
  
Perhaps one option would be to use paint_listener.js: wait for the page to load, then change something (e.g. the background color) and use paint_listener.js to wait for the paint to complete. By then we should have decided that the plugin is visible.

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Wait for paint flushs (obsolete)
        — 
          — Splinter Review
      

    


  
Use paint_listener to wait for paint flushs and thus visibility.
Fixes the issue locally for me, so i'm hopeful.

https://tbpl.mozilla.org/?tree=Try&rev=7b404774b4d7

    
  

        Attachment #684779 -
        Attachment is patch: true

    
    

    
      
  

    
    

    
      
  
Whiteboard: [orange]

    
  

        Attachment #680603 -
        Flags: checkin+

    
    

    
  
Comment on attachment 684779 [details] [diff] [review]
Wait for paint flushs

Looks good on try.

Requesting review for this follow-up, hopefully the last one.

        Attachment #684779 -
        Flags: review?(joshmoz)

    
    

    
  
Comment on attachment 684779 [details] [diff] [review]
Wait for paint flushs

Review of attachment 684779 [details] [diff] [review]:
-----------------------------------------------------------------

Please add code comments in the test explaining why you're doing this.

Do we really need to copy paint_listener.js? I'm not saying it isn't reasonable to do so, and you don't need to go way out of your way to avoid it, just wondering.

        Attachment #684779 -
        Flags: review?(joshmoz) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Wait for paint flushs (obsolete)
        — 
          — Splinter Review
      

    


  
https://tbpl.mozilla.org/?tree=Try&rev=427878feca29

        Attachment #684779 -
        Attachment is obsolete: true

        Attachment #688380 -
        Flags: review?

    
    

    
  
Comment on attachment 688380 [details] [diff] [review]
Wait for paint flushs

roc, are you ok with moving the paint listener over to the mochitest so it can be shared across modules?

        Attachment #688380 -
        Flags: review? → review?(roc)

    
    

    
  
(In reply to Georg Fritzsche [:gfritzsche] from comment #69)
> roc, are you ok with moving the paint listener over to the mochitest so it
> can be shared across modules?

Forgot to add: Just flagging for review for the layout/ parts.

    
    

    
  
Comment on attachment 688380 [details] [diff] [review]
Wait for paint flushs

robcee, can you review the testing/mochitests parts?
This should match the earlier IRC chat.

        Attachment #688380 -
        Flags: review?(rcampbell)

    
    

    
      
  

    
    

    
  
Comment on attachment 688380 [details] [diff] [review]
Wait for paint flushs

Review of attachment 688380 [details] [diff] [review]:
-----------------------------------------------------------------

That's fine, but use hg rename to move the file so we don't lose history.

        Attachment #688380 -
        Flags: review?(rcampbell) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
Thanks for catching the plain copy/remove roc, i completely missed that.

robcee, any objections?

        Attachment #688380 -
        Attachment is obsolete: true

        Attachment #688380 -
        Flags: review?(roc)

        Attachment #688733 -
        Flags: review?(rcampbell)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 688733 [details] [diff] [review]
Wait for paint flushs, v4

sorry I didn't see this sooner.

r+

        Attachment #688733 -
        Flags: review?(rcampbell) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/903c331107c2
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla19 → mozilla20

    
    

    
      
  

    
  
Depends on: 824042

    
  
No longer depends on: 824042

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: