Closed Bug 756844 Opened 12 years ago Closed 12 years ago

Intermittent test_browserFrame9.html | First Screenshot is not blank and/or | Screenshots differ

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
All
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: philor, Assigned: daleharvey)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 5 obsolete files)

Change test strategy for screenshot API
3.42 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review 
https://tbpl.mozilla.org/php/getParsedLog.php?id=11897835&tree=Mozilla-Inbound
Rev3 Fedora 12x64 mozilla-inbound pgo test mochitests-3/5 on 2012-05-19 22:00:37 PDT for push 8f1c93b5b549
slave: talos-r3-fed64-051

3413 INFO TEST-START | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html
creating 1!
[TabChild] SHOW (w,h)= (0, 0)
loading about:blank, 1
loading data:text/html,<html><body%20style="background:green">hello</body></html>, 1
loading data:text/html,<html><body%20style="background:blue">hello</body></html>, 1
3414 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | First Screenshot is not blank
3415 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | Screenshots differ
3416 INFO TEST-END | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | finished in 69ms

https://tbpl.mozilla.org/php/getParsedLog.php?id=11891448&tree=Services-Central
Rev3 Fedora 12x64 services-central debug test mochitests-3/5 on 2012-05-19 09:52:44 PDT for push a0d9e03ba59f
slave: talos-r3-fed64-016

3413 INFO TEST-START | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html
++DOMWINDOW == 45 (0x3d44198) [serial = 750] [outer = 0xfeada0]
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 160: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 170: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 160: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 160: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 160: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getBoolPref]
[Child 2324] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsWebBrowser.cpp, line 407
[Child 2324] WARNING: NS_ENSURE_TRUE(domWindow) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 123
[Child 2324] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsWebBrowser.cpp, line 407
[Child 2324] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsWebBrowser.cpp, line 407
[Child 2324] WARNING: NS_ENSURE_TRUE(domWindow) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 123
[Child 2324] WARNING: NS_ENSURE_TRUE(inBrowser) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 119
[Child 2324] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsWebBrowser.cpp, line 407
[Child 2324] WARNING: NS_ENSURE_TRUE(domWindow) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 123
[Child 2324] WARNING: NS_ENSURE_TRUE(inBrowser) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 119
[Child 2324] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsWebBrowser.cpp, line 407
[Child 2324] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsWebBrowser.cpp, line 407
[Child 2324] WARNING: NS_ENSURE_TRUE(domWindow) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 123
[Child 2324] WARNING: NS_ENSURE_TRUE(inBrowser) failed: file /builds/slave/srv-cen-lnx64-dbg/build/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 119
JavaScript error: chrome://specialpowers/content/SpecialPowersObserverAPI.js, line 170: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]
creating 1!
[TabChild] SHOW (w,h)= (0, 0)
[Child 2324] WARNING: nsWindow::GetNativeData not implemented for this type: file /builds/slave/srv-cen-lnx64-dbg/build/widget/xpwidgets/PuppetWidget.cpp, line 611
++DOCSHELL 0x2856860 == 9 [id = 9]
++DOMWINDOW == 24 (0x25a3a88) [serial = 24] [outer = (nil)]
++DOMWINDOW == 25 (0x26260b8) [serial = 25] [outer = 0x25a3a10]
loading about:blank, 1
loading data:text/html,<html><body%20style="background:green">hello</body></html>, 1
loading data:text/html,<html><body%20style="background:blue">hello</body></html>, 1
++DOMWINDOW == 26 (0x2750708) [serial = 26] [outer = 0x25a3a10]
[TabChild] Update Dimensions to (x,y,w,h)= (0d, 25d, 780d, 1112d) and move to (w,h)= (0d, 0d)
[TabChild] Update Dimensions to (x,y,w,h)= (0d, 25d, 780d, 1112d) and move to (w,h)= (300d, 150d)
3414 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | First Screenshot is not blank
3415 INFO TEST-PASS | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | Screenshots differ
3416 INFO TEST-END | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | finished in 196ms
Ok, Sorry about that, and thanks for the heads up, will get this fixed asap

Justin, Where is the best place for me to learn about the exact ordering of the various events? the only way this can fail is if painting occurs after mozloadend
> Justin, Where is the best place for me to learn about the exact ordering of the various events? 
> the only way this can fail is if painting occurs after mozloadend

Yes, right.

Reftest has to solve this problem somehow.  See layout/tools/reftest/reftest{,-content}.js.  It looks...complicated.

I wonder if we could synchronously force a paint before we take a screenshot.  That would be nice.

An alternative would be to force a repaint after mozbrowserloadend.  Then we'd be guaranteed to get a MozAfterPaint.

Otherwise, maybe you could hack the test by injecting a script into the iframe that listens to mozpaint and sends you a message when it happens...  The trick is, you want to take a screenshot after the first mozpaint which happens after X, where X indicates that we've finished building up the page, in some sense.  And I'm not sure what X is!

dholbert, do you have any thoughts here?  This test is doing canvas.drawWindow inside a child process.  Right now it's doing that after onload, but that's clearly not right.
the X we need to wait after right now is iframe.src = 'some html', in future it may be any of iframe.go / iframe.goBack etc

We need any event that is fired from all of these, and doesnt return to the userland until we have set the _painted flag

The additional methods (go / reload etc) we will be implementing in the wrapper and so can set the painted flag synchronously, the only issue is whether we still support .setAttribute('src' on the iframe, and if so how do I hook into that to invalidate the flag

The original STATE_START method might be good for that, even better would be the onLocationChange (we dont care if we invalidate _painted too often), the only question is whether they are guaranteed to be fired before .src = returns, which since we are on a single process seem like a safe assumption, but at the same time it still doesnt feel clean.

But beginning to get a bit lost trying to find the right + clean solution, a lot of the tests look to be waiting for mozpaint manually, some are even just setTimeouting
> the X we need to wait after right now is iframe.src = 'some html'

That's the right idea, but it's not quite right.

iframe.src=foo can trigger an asynchronous operation.  (In fact, I think it almost always does, even for data: URIs.)  So suppose your iframe is at a page with some animated GIFs and you do iframe.src=foo.  There's no reason that the next mozpaint after setting iframe.src will correspond to foo and not to the page with the animated GIFs.
Ok, how about getScreenshot stays as is and takes a screenshot at the moment it is called

And in the test we do the set flag on loadstart, wait for at least a loadend + aftermozpaint, and we enable aftermozpaint in b2g? Its disabled for a security leak, but the leak is much less than we are already exposing via getScreenshot anyway

or we could just enable aftermozpaint for the tests, but I would kind a like to get screenshots consistently in the browser app as well.
I forgot to mention, this is mostly copying android firefox, and not entirely sure loadend + aftermozpoint is required, it looks like the first aftermozpaint proceeding loadstart will do
I'm out of my depth here; we could really use some help from the layout team understanding how this is supposed to work.  (Looks like I tried to cc dholbert earlier but failed.)

Could you point me to where you heard about disabling mozafterpaint?  I understand disabling it for content, but we should be able to leave it enabled for chrome...

> And in the test we do the set flag on loadstart, wait for at least a loadend + aftermozpaint, and 
> we enable aftermozpaint in b2g?

I haven't done a good job explaining why I think this may be problematic.  Let me try again:

mozafterpaint is pretty asynchronous; exactly how asynchronous it is, I don't know.  But we know because this bug exists that sometimes our document isn't painted until after onload, and other times our document is painted before onload.  (Our mozbrowserloadend event happens a bit after onload.)

The problem is, I think we may paint the page in an incomplete state (*).  We could paint after the load starts, but before everything is set up in your test page.  What we really want is a paint after onload, at which point we know everything is set up (**).  But of course we can't guarantee that we will *get* a paint event after onload, because we might have painted everything before then!  (Unless we put an animation in the page, was the hack I suggested.)

This would all be easy if we could just say "paint now; I'll wait."  Or even "paint sometime soon, and give me a callback once you've done so."  Either one of those would fix the test and the screenshot API.  Maybe there's an existing API we can frob.

(*) But maybe roc can explain this, because I actually have no idea.
(**) At least, everything in the DOM is set up at this point.  I presume layout is in sync?
Can you use DOMWindowUtils::isMozAfterPaintPending?
Basically I think you should
a) install MozAfterPaint listener on the toplevel browser window
b) wait for onload to fire
c) then check isMozAfterPaintPending. If it is, wait for the next MozAfterPaint event to fire and take the screenshot. Otherwise, take the screenshot now.
Sorry I wasnt clear, I meant to propose that we allow content to listen to mozafterevent, specifically the parent window

> iframe.addEventListener('MozAfterPaint', ....

Its exposing the security leak that mozafterpaint was disabled (on content) for, but that leak is a subset of being able to take screenshots anyway (https://bugzilla.mozilla.org/show_bug.cgi?id=608030)

I think its wrong for us to be doing weird mechanics to determine when is the best time to take a screenshot, but if we dont do it the caller needs to have the information to make the decision
s/to listen to mozafterevent/to listen to mozafterpaint
I don't know what your test is supposed to be testing, so I don't know when you need to grab your screenshot.

If you control the pages being loaded, maybe you can have those pages signal the container to take a screenshot when they're ready, using postMessage or something like that?
The test is testing that when we make changes to the iframes content, we see those changes in subsequent screenshots, we want to be able to keep an uptodate screenshot (so that we dont have screenshots of loading pages for example)

We dont control the pages being loaded, this is being added for the browser in b2g, that will load arbitrary webpages
What does it mean to have "an up to date screenshot"? The DOM "load" event has fired?
No, just that its contents has changed, if I take a screenshot of a page and its contents completely changes, I would like the ability to know that the screenshot is outdated. That is a seperate issue to whether to block the screenshot till load, however it would have also fixed this problem.

But sorry for drawing this bug out, basically the assumptions of the test are wrong, getScreenshot works as it should do, and if we want the ability to listen for updates or block for the first paint after load (we might not want to) then they are seperate issues, attaching an updated test that fixes the tests assumptions.

        Attachment #625980 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
Comment on attachment 625980 [details] [diff] [review]
fix expectation of getScreenshot api

The point with the animation hack was that you'd take the screenshot after the first paint *after loadend*.  It looks like here you're taking the screenshot after the first paint after setting the src.

I don't see why roc's suggestion in comment 28 doesn't work.

        Attachment #625980 -
        Flags: review?(justin.lebar+bug) → review-

    
    

    
  


  
Sorry assumed the paintpending comment was meaning we go back to blocking the getScreenshot method until page load, which isnt going to work.

For blocking the call to getScreenshot, it works well though, same idea as my last patch but cleaner

        Attachment #625980 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 625995 [details] [diff] [review]
fix expectation of getScreenshot api

> SimpleTest.waitForExplicitFinish();
> 
>+var iframeScript = function() {
>+  const CI = Components.interfaces;

Nit: We idiomatically use |Ci|.

>+  function painted() {
>+    sendAsyncMessage('test:mozpainted');
>+  }
>+  content.addEventListener("MozAfterPaint", painted, false);
>+  var utils = content.QueryInterface(CI.nsIInterfaceRequestor)
>+    .getInterface(CI.nsIDOMWindowUtils);
>+  if (!utils.isMozAfterPaintPending) {
>+    content.removeEventListener("MozAfterPaint", painted, false);
>+    sendAsyncMessage('test:mozpainted');
>+  }

I'd prefer if we did this in the opposite order -- don't add the event listener if we're just going to remove it -- but whatever.  :)

Let's push this to try and trigger a bunch of test runs.

        Attachment #625995 -
        Flags: review+

    
    

    
  


  
Addressed those fixes

Will push to try

        Attachment #625995 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  
Hardware: x86_64 → All

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
I triggered a bunch of Linux64 opt moth3 runs.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Good thing you triggered those tests

So for this to fail, either paintPending is wrong, or ctx.drawWindow isnt picking up the painted page

I will try to go a bit deeper into what went wrong, I would suspect ctx.drawWindow, but I am beginning to get a bit stuck as this is impossible to reproduce locally and I dont have much background in this code

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
jlebar, looking futher into this is getting pretty hairy, and its only supposed to be a smoke test to make sure that the current display of the page is captured, not to test the synchronicity of the paint event etc

How about we trigger another bunch of runs and as long as 

> ok(screenshots[0] !== screenshots[1], 'Screenshots differ');
  
passes then we are good, I can write up a new test + file a bug for ctx.drawWindow + mozpaintafter if needed

    
    

    
  
> How about we trigger another bunch of runs and as long as 
> 
> > ok(screenshots[0] !== screenshots[1], 'Screenshots differ');
>   
> passes then we are good, I can write up a new test + file a bug for
> ctx.drawWindow + mozpaintafter if needed

Fine by me.  You should be able trigger the extra tests from TBPL; once the Linux64-opt mochitest-3 starts running (turns from light gray to dark gray), click the 3 and then click the plus sign in the bottom left box a bunch of times.  (Or come find me.)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
I have created a bug on the dom/layout/view rendering @ https://bugzilla.mozilla.org/show_bug.cgi?id=758081

There was one complete failure during the last run but that looks like a build bot issue, the rest were all the initial blank screenshot

Do you want me to push this to try as well?

        Attachment #626006 -
        Attachment is obsolete: true

        Attachment #626656 -
        Flags: review?(justin.lebar+bug)

    
    

    
      
  

    
    

    
  
pushed to try anyway

https://tbpl.mozilla.org/?tree=Try&rev=36c4d7773c27

will trigger an extra bunch of 3's

    
    

    
  
Comment on attachment 626656 [details] [diff] [review]
fix expectation of getScreenshot api

Let me know when you're satisfied with the try run and we can check this in.  (I'm also OK if you want to cancel the try run and check this in as-is; it seems pretty likely that removing this line should fix the intermittent orange.)

        Attachment #626656 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
      
  

    
    

    
  
I was pushing to try for the setVisible patch, just figured I would do this one at the same time, so yeh I am happy for this to get checked in. Cheers

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/30094d3d116e
Assignee: nobody → dale
Target Milestone: --- → mozilla15

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/30094d3d116e
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Just hit what looks like this bug on m-i:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=12032580&tree=Mozilla-Inbound
3411 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/browser-frame/test_browserFrame9.html | Screenshots differ

Reopening. (apologies if I'm misunderstanding)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
      
  

    
    

    
  
Nope that is that this same bug that was assumed fix

I am somewhat lost on how to test this now, we know the functionality works as we want it to, however testing it reliably without making certain assumptions on mozpaintafter seems impossible

    
    

    
  
How exactly does the getScreenshot API work? I see that you call Services.DOMRequest.createRequest(frameElement.ownerDocument.defaultView) but I have no idea what that does.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
DomRequest just returns a promise to the user so they can receive the result of the call

getScreenshot is called on the chrome process, which sends a message to the content process to use a ctx.drawWindow to draw the current window to a canvas, the image data is returned

This test is 

1. setting iframe to a blue page
2. waiting for the page to fire loadend
3. waiting for the page to fire a mozafterpaint (if one is pending)
4. using ctx.drawWindow

Then repeating that for a green page.

I expected that by waiting for both onload and mozafterpaint that the current screen will be fully painted and the image data returned will be a blue and a green page, what is actually happening is that the image data returned is blank

I think the bug is in ctx.drawWindow, and have filed a bug on it, https://bugzilla.mozilla.org/show_bug.cgi?id=758081

I dont think I have much of a choice but to find out where that bug is to fix this test, it just goes a lot lower than I am familiar with

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  


  
Will push to try and trigger as many runs of 3 that I can :)

        Attachment #626656 -
        Attachment is obsolete: true

        Attachment #628168 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
>+        content.document.defaultView.setTimeout(function() {
>+          iframe1.getScreenshot().onsuccess = screenshotLoaded;
>+        }, 200);

setTimeout(0) should work just as well; no harm in spinning the CPU.  We try not to rely on setTimeout(N) for N > 0 because, for example, someone could try to change the mochitest timeout to 1s, which would be bad here...

>+    var attempts = 10;

We shouldn't have a max-attempts variable; it's just asking for a less-intermittent orange.  The test framework will kill the test if it runs too long, so we should just poll infinitely and rely on eventually being killed.

    
    

    
  
Comment on attachment 628168 [details] [diff] [review]
Change test strategy for screenshot API

r=me with those changes.

        Attachment #628168 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
      
  

    
    

    
  


  
I am waiting on doing a full rebuild so havent tested yet

        Attachment #628168 -
        Attachment is obsolete: true

        Attachment #628545 -
        Flags: review?(justin.lebar+bug)

    
    

    
  
Ok tested, works here, want another try run or is this good?

    
    

    
  
Comment on attachment 628545 [details] [diff] [review]
Change test strategy for screenshot API

I don't think we need another try run; this is good.  Want me to push it?

        Attachment #628545 -
        Flags: review?(justin.lebar+bug) → review+

    
    

    
  
Yup good by me, cheers

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/963ebb01fcba
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED

    
    

    
      
  
Whiteboard: [orange]
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: