Closed Bug 832920 Opened 7 years ago Closed 7 years ago

CSS Transitions don't start in chrome iframes after swapFrameLoaders is called

Categories

(Core :: Document Navigation, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox21 --- fixed
firefox22 --- fixed

People

(Reporter: paul, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
I'm working on a test case.
Blocks: 820343, 832918
Summary: CSS Transititions don't start in chrome iframes after swapFrameLoaders is called → CSS Transitions don't start in chrome iframes after swapFrameLoaders is called
Blocks: 814641
cc'ing some people who managed to help before with other swapFrameLoaders related bugs.

Here is a description of the problem:

In the developer tools, we let the user undock the toolbox into its own window. We use swapFrameLoaders to do that seamlessly.

After swapFrameLoaders has been used, some XUL elements are not painted correctly if they rely on a CSS transition.

An example (can be reproduced on Windows):
- start the inspector (in a new tab, right click anywhere, click on "inspect element")
- on the right of the inspector, there is a panel with the CSS rules
- move mouse over a property, see a checkbox appearing with a subtle transition on opacity
- undock the toolbox (double-window button next to the close button)
- move mouse over a property, the checkbox doesn't appear

CSS code of this checkbox: http://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/devtools/csshtmltree.css#204

We have other examples of UI elements not being correctly painted (but we can interact with these invisible elements). See blocked bugs. They all use transitions.

I didn't manage to build a reduced test case yet. Something as simple as that https://gist.github.com/4585528 doesn't appear to be enough to reproduce the problem.
OS: Linux → Windows 8
Is this perhaps a regression from refreshdriver changes?
I don't think this is a regression. I think it never worked.
Blocks: 830911
I would really need some help from anyone from platform would could help me to demystify this problem.
Hmm.  The steps from comment 2 work for me on Mac, but maybe the rules are different there.

Paul, if I write up a test patch, can you try it?
Attached patch Possible fix (obsolete) — Splinter Review
Attachment #705088 - Flags: feedback?
Attachment #705088 - Flags: feedback? → feedback?(paul)
Comment on attachment 705088 [details] [diff] [review]
Possible fix

>+  // Now that we've set up the parent docshells and documents
>+  // correctly, hook up the right refresh drivers too.
>+  ReinitRefreshDriver(ourChildDocument);
>+  ReinitRefreshDriver(otherChildDocument);
If it helps, we have ourShell and otherShell available.
Thank you. I'll test this patch asap.
(In reply to Boris Zbarsky (:bz) from comment #7)
> Created attachment 705088 [details] [diff] [review]
> Possible fix

As soon as swapFrameLoaders is called, crash.
Attachment #705088 - Flags: feedback?(paul) → feedback-
> As soon as swapFrameLoaders is called, crash.

Hmm.  Not in general, but yes for the devtools case, because we end up with what looks like a stale presshell pointer in the refresh driver.

That's actually pretty annoying.  Certainly when I was initially writing swapFrameLoaders it was meant to only work on self-contained things, which means non-chrome iframes.  We loosened up that restriction, but it's not obvious we should have.  :(

I'll think about whether there's a sane way to resync the refresh driver state somehow.  :(
Blocks: :PaulFx21
Boris, do you think this is something we could fix for Firefox 21 (now, or in Aurora)? Asking, because otherwise, we need to remove all the CSS transition from our code.
Yikes.

I'll take a look at this today, but the patch, if any, will be incredibly scary, I bet.  We have all sorts of things that assume the refresh driver for a given presshell is immutable...
Attached patch Hackery that might not crash (obsolete) — Splinter Review
Paul, does _this_ happen to fix the bug on Windows?  I'm still trying to understand whether the refresh driver thing is all there is to this or whether there is something else...

Note that this patch is certainly not correct; it just manages to not crash the way the previous one did.
Assignee: nobody → bzbarsky
Flags: needinfo?(paul)
Thanks Boris for the quick patch, and sorry for the late reply.

(In reply to Boris Zbarsky (:bz) from comment #14)
> Created attachment 714031 [details] [diff] [review]
> Hackery that might not crash

It doesn't crash.

> Paul, does _this_ happen to fix the bug on Windows?

No.

More precise STR:

- make sure the toolbox starts undocked (open the inspector (in a web page, right click on an element->inspect), undock it, close it)
- open the inspector (it starts in its own window)
- don't click anywhere in the toolbox
- dock the toolbox by clicking on the dock icon
- move your mouse over the Rule sidebar

Expected: checkbox appears
What happens: no checkboxes show up
Flags: needinfo?(paul)
Thanks for the more precise STR.  Now I can reproduce the bug locally!

I'm not sure this has to do with the refresh driver per se.  I also see two-finger drag in the rules sidebar not scrolling, and clicking on the scrollbars in the sidebar and inspector doesn't work.  Clicking other things _does_ work.

Now that I can reproduce I can at least try some experiments to pin this down.
Yeah, definitely refresh driver issue.  If I force this particular iframe into having its own refresh driver, things work.  And that makes sense with the str from comment 15: the refresh driver from the old window is dead there.

I have no idea why the "Hackery" patch didn't work; presumably I didn't move something that really matters from the old driver to the new.

So my current plan is to add a way for <xul:iframe/browser> to opt in to having a separate refresh driver even though it's not a root of a same-type subtree, then opt in the iframes involved here.  And file a followup to require the opt-in for the swap to succeed (will likely need addon sdk changes).

Timothy, thoughts?
Flags: needinfo?(tnikkel)
(In reply to Boris Zbarsky (:bz) from comment #17)
> So my current plan is to add a way for <xul:iframe/browser> to opt in to
> having a separate refresh driver even though it's not a root of a same-type
> subtree, then opt in the iframes involved here.  And file a followup to
> require the opt-in for the swap to succeed (will likely need addon sdk
> changes).

That seems reasonable if we need to support this use case. Hopefully this "opting in" will only get used where it is really needed and won't spread via cargo-culting.
Flags: needinfo?(tnikkel)
(In reply to Timothy Nikkel (:tn) from comment #18)
> Hopefully this
> "opting in" will only get used where it is really needed and won't spread
> via cargo-culting.

Can you explain that please? Just want to make sure that on the frontend side we are not (or we won't be) doing anything wrong.
(In reply to Paul Rouget [:paul] from comment #19)
> (In reply to Timothy Nikkel (:tn) from comment #18)
> > Hopefully this
> > "opting in" will only get used where it is really needed and won't spread
> > via cargo-culting.
> 
> Can you explain that please? Just want to make sure that on the frontend
> side we are not (or we won't be) doing anything wrong.

I just meant that only iframes that need their own refresh driver to fix this bug should opt in and that hopefully not every iframe will opt in "just in case".
Whiteboard: [need review]
Attachment #705088 - Attachment is obsolete: true
Attachment #714031 - Attachment is obsolete: true
Comment on attachment 716665 [details] [diff] [review]
Add a way for chrome iframes to opt into having a separate refresh driver (e.g. if they plan to be moved between windows) and make devtools use that opt-in.

Looks good.

forceOwnRefreshDriver sounds like a better name to me, but I'm fine with either way.

Also, do we want to require the parent to be chrome? I guess the XUL check does that already.
Attachment #716665 - Flags: review?(tnikkel) → review+
> forceOwnRefreshDriver sounds like a better name to me

Done.

> I guess the XUL check does that already.

Pretty much, yes.
Comment on attachment 716665 [details] [diff] [review]
Add a way for chrome iframes to opt into having a separate refresh driver (e.g. if they plan to be moved between windows) and make devtools use that opt-in.

It works. Thanks a lot for that.

Can we consider getting this in Firefox 21?
Attachment #716665 - Flags: review?(paul) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/01145aac2536
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla22
Comment on attachment 716665 [details] [diff] [review]
Add a way for chrome iframes to opt into having a separate refresh driver (e.g. if they plan to be moved between windows) and make devtools use that opt-in.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 577607 and changes to devtools UI
User impact if declined: Devtools UI doesn't behave properly after docking or
   undocking the tools.
Testing completed (on m-c, etc.): Passes manual test of the behavior.
Risk to taking this patch (and alternatives if risky): Low risk: should only
   affect the devtools UI.
String or UUID changes made by this patch: None
Attachment #716665 - Flags: approval-mozilla-aurora?
Unfortunately, this was backed out due to one of the changesets in the push making browser_dbg_bug723069_editor-breakpoints.js fail frequently on Windows and OSX opt builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca34c11bf55d

https://tbpl.mozilla.org/php/getParsedLog.php?id=19995636&tree=Mozilla-Inbound

11:12:16     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | editor.getBreakpoints().length is correct
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | remove the second breakpoint using the mouse
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Console message: [JavaScript Warning: "Use of attributes' specified attribute is deprecated. It always returns true." {file: "chrome://browser/content/orion.js" line: 6341}]
11:12:16     INFO -  TEST-INFO | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Console message: [JavaScript Warning: "Use of removeAttributeNode() is deprecated. Use removeAttribute() instead." {file: "chrome://browser/content/orion.js" line: 6342}]
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:17     INFO -  DBG-FRONTEND: Updating the DebuggerView editor: http://example.com/browser/browser/devtools/debugger/test/test-script-switching-02.js @ 6, flags: ({noSwitch:true})
11:12:46  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | Test timed out
11:12:46  WARNING -  This is a harness error.
11:12:46     INFO -  args: ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png', '/var/folders/qd/srwd5f710sj0fcl9z464lkj00000gn/T/mozilla-test-fail_GzeI8j']
11:12:48     INFO -  SCREENSHOT: <see log>
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView
11:12:48     INFO -  DBG-FRONTEND: Destroying the ToolbarView
11:12:48     INFO -  DBG-FRONTEND: Destroying the OptionsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the ChromeGlobalsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the SourcesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the FilterView
11:12:48     INFO -  DBG-FRONTEND: Destroying the FilteredSourcesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the StackFramesView
11:12:48     INFO -  DBG-FRONTEND: Destroying the BreakpointsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the WatchExpressionsView
11:12:48     INFO -  DBG-FRONTEND: Destroying the GlobalSearchView
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView window
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView panes
11:12:48     INFO -  DBG-FRONTEND: Destroying the DebuggerView editor
11:12:48     INFO -  DBG-FRONTEND: SourceScripts is disconnecting...
11:12:48     INFO -  DBG-FRONTEND: StackFrames is disconnecting...
11:12:48     INFO -  DBG-FRONTEND: ThreadState is disconnecting...
11:12:48     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been added
11:12:48     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of breakpoints have been removed
11:12:48  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | correct number of editor breakpoint changes - Got 3, expected 4
11:12:48  WARNING -  This is a harness error.
11:12:48     INFO -  Stack trace:
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: test_is :: line 486
11:12:48     INFO -      JS frame :: chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js :: <TOP_LEVEL> :: line 295
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: Tester_nextTest :: line 250
11:12:48     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: <TOP_LEVEL> :: line 419
11:12:48     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
11:12:48     INFO -  INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/debugger/test/browser_dbg_bug723069_editor-breakpoints.js | finished in 30099ms
Comment on attachment 716665 [details] [diff] [review]
Add a way for chrome iframes to opt into having a separate refresh driver (e.g. if they plan to be moved between windows) and make devtools use that opt-in.

got backed out of m-i, clearing approval
Attachment #716665 - Flags: approval-mozilla-aurora?
OK, it looks like this patch in fact triggers the orange described in comment 27.

The main effect of this patch is that it gives the entire devtools iframe a separate refresh driver, which can change how timing of things in the devtools iframe matches up with timing of things in the main browser window.

It doesn't look like anything relevant to the above test uses requestAnimationFrame, so I'm not sure what _is_ going on.  Presumably some other aspect of timing is tickling something in the test.

Mihai, do you know who might have the time to look into what happens with the debugger test with this patch?
Flags: needinfo?(mihai.sucan)
Whiteboard: [need debugger test orange sorted out]
Target Milestone: mozilla22 → ---
(In reply to Boris Zbarsky (:bz) from comment #29)
> OK, it looks like this patch in fact triggers the orange described in
> comment 27.
> 
> The main effect of this patch is that it gives the entire devtools iframe a
> separate refresh driver, which can change how timing of things in the
> devtools iframe matches up with timing of things in the main browser window.
> 
> It doesn't look like anything relevant to the above test uses
> requestAnimationFrame, so I'm not sure what _is_ going on.  Presumably some
> other aspect of timing is tickling something in the test.
> 
> Mihai, do you know who might have the time to look into what happens with
> the debugger test with this patch?

I spent time today looking into this failure and I also did some changes in the test, in the hope I can get it working. I pushed a patch to try:

https://tbpl.mozilla.org/?tree=Try&rev=5c4d891eca44

The place where the test stops is at synthesizeMouse() which is expected to work, but it fails to work. This is a problem that I have previously seen, it's not rare. Sometimes when writing mochitests synthesizeMouse() fails to work, for unexplained reasons. I usually have to add executeSoon() or find other ways to do the call later. It happens even with much simpler tests and cases where I have a straightforward wait-for-load/open foo, then click.

Is there a way to determine where the synthesizeMouse() event went? To better understand why editorElement doesn't get the click event.
Flags: needinfo?(mihai.sucan)
Is the situation that synthesizeMouse is called from a load event handler? We unsuppress painting just after we dispatch the load event, so the mouse event will not find the page contents that way. Just doing the mouse event off a setTimeout from the load event should work for that.
(In reply to Timothy Nikkel (:tn) from comment #31)
> Is the situation that synthesizeMouse is called from a load event handler?
> We unsuppress painting just after we dispatch the load event, so the mouse
> event will not find the page contents that way. Just doing the mouse event
> off a setTimeout from the load event should work for that.

Afaik, this is not the case here, but thanks for the suggestion - this is good to know.
Are you calling synthesizeMouse directly on the element you care about (as opposed to a frame that contains that element)?
Test passes for me and it's green on try servers:
https://tbpl.mozilla.org/?tree=Try&rev=5c4d891eca44

Please check if this works for you. If yes, the refresh driver change patch can land.
Attachment #719093 - Flags: feedback?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #33)
> Are you calling synthesizeMouse directly on the element you care about (as
> opposed to a frame that contains that element)?

The test sends the click to the iframe element that contains the element I care about. It's not practical to pin-point within the Orion editor UI which element to click on, as that can change any time Orion's updated. This worked well, until now. Should we avoid doing things like this?
Ah.  So _that_ could certainly be affected by the situation with refresh drivers and timing.  Sending a mouse event to some element will flush layout so that it's up to date on that element's document and its ancestors, but NOT on descendant documents.  So if the descendant document's layout is not up to date, you can get events targeted to the wrong things, I expect...  Does explicitly flushing layout on the contentDocument help?  If so, that would validate the hypothesis that not-up-to-date layout on the child is the problem.
I guess the other question is... if you don't know which element in the child document you want, how are you deciding where on the iframe to send the mouse event (i.e. its coordinates)?
Comment on attachment 719093 [details] [diff] [review]
debugger test fix

Oh, as far as the try run goes, I triggered a few more test runs to see how those go.  Past that, I can't tell whether this patch is right or not, obviously....
Attachment #719093 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky (:bz) from comment #36)
> Ah.  So _that_ could certainly be affected by the situation with refresh
> drivers and timing.  Sending a mouse event to some element will flush layout
> so that it's up to date on that element's document and its ancestors, but
> NOT on descendant documents.  So if the descendant document's layout is not
> up to date, you can get events targeted to the wrong things, I expect... 
> Does explicitly flushing layout on the contentDocument help?  If so, that
> would validate the hypothesis that not-up-to-date layout on the child is the
> problem.

I see. I'm learning new stuff here. So it might make sense to synthesize the click at iframe.contentDocument? (if I'm correctly understanding what you suggest)


(In reply to Boris Zbarsky (:bz) from comment #37)
> I guess the other question is... if you don't know which element in the
> child document you want, how are you deciding where on the iframe to send
> the mouse event (i.e. its coordinates)?

Font size is fixed, hard coded values for now. There's a bug about hidpi support on Macs (can't find the bug number now). That can be fixed by using Orion's API to determine line heights, so we can correctly pick the coordinates.
> So it might make sense to synthesize the click at iframe.contentDocument

Maybe, yes.  Or at least doing iframe.contentDocument.documentElement.getBoundingClientRect() before synthesizing the click.

> Font size is fixed, hard coded values for now

Ah.  Yeah, that obviously doesn't flush layout anywhere....  Using the Orion API is likely to do the needed layout flushes.

Could the hidpi issue be why this test always fails for me?

In any case, try is looking good.  Mihai, I assume you patch needs review, right?
(In reply to Boris Zbarsky (:bz) from comment #40)
> In any case, try is looking good.  Mihai, I assume you patch needs review,
> right?

It's a test-only change. I can ask for review from you or Panos. Going to pick Panos - he knows devtools code.
Good call; I'm not a qualified reviewer for that patch.  ;)
Comment on attachment 719093 [details] [diff] [review]
debugger test fix

This is the patch that seems to fix the test failure for this bug. Try push was green. What I changed is only how the test starts - I was not too happy how debug_tab_pane() prepares this test to run.

An improved fix for the test should be implemented in bug 845616 or here, see comment 36. Thoughts?
Attachment #719093 - Flags: review?(past)
Comment on attachment 719093 [details] [diff] [review]
debugger test fix

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

In general I'd prefer to fix debug_tab_pane if it is indeed buggy, but let's do this now and find a better fix in bug 845616.
Attachment #719093 - Flags: review?(past) → review+
(In reply to Panos Astithas [:past] from comment #44)
> Comment on attachment 719093 [details] [diff] [review]
> debugger test fix
> 
> Review of attachment 719093 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general I'd prefer to fix debug_tab_pane if it is indeed buggy, but let's
> do this now and find a better fix in bug 845616.

Thanks for the review. I avoided touching debug_tab_pane to not introduce intermittent failures in other debugger tests. It's much harder to predict the effect of changes in that function for all of the tests.
Let's try that again:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/0feef91f29f1
   https://hg.mozilla.org/integration/mozilla-inbound/rev/f965c8e4a24f
Whiteboard: [need debugger test orange sorted out]
Target Milestone: --- → mozilla22
Unfortunately, I had to backout (again!) because something in this push caused a spike in mochitest-a11y assertions.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb572a342efa

https://tbpl.mozilla.org/php/getParsedLog.php?id=20189591&tree=Mozilla-Inbound

A quick skim of the log shows them to be of the type:
###!!! ASSERTION: bad size recorded: 'aInstanceSize == 0 || entry->GetClassSize() == aInstanceSize', file ../../../xpcom/base/nsTraceRefcntImpl.cpp, line 441
dbaron, weren't you looking into those recently?
Flags: needinfo?(dbaron)
Can we have this in Firefox 21?
Comment on attachment 716665 [details] [diff] [review]
Add a way for chrome iframes to opt into having a separate refresh driver (e.g. if they plan to be moved between windows) and make devtools use that opt-in.

Oh, right, the approval requests got removed... Let's try that again.
Attachment #716665 - Flags: approval-mozilla-aurora?
Comment on attachment 719093 [details] [diff] [review]
debugger test fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): General test flakiness
User impact if declined: Can't land the other patch in this bug
Testing completed (on m-c, etc.): Green on tbpl.
Risk to taking this patch (and alternatives if risky): It's a test-only change,
   so the main risk is of intermittent orange.
String or UUID changes made by this patch: None.
Attachment #719093 - Flags: approval-mozilla-aurora?
Attachment #716665 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #719093 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
See Also: → 1542468
You need to log in before you can comment on or make changes to this bug.