1116714 - The custom content container should not have a frame when it's not used

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Mats Palmgren (inactive)]

STEPS TO REPRODUCE
1. firefox -layoutdebug about:blank
2. Dump -> Frames

ACTUAL RESULTS
The ViewportFrame has a fixed pos child frame.  This is for the
"custom content container" added in bug 1020244.

I find this rather annoying when trying to debug frame tree issues.
It's also a memory usage / performance issue.

I think we should create this frame only when the custom content container
actually has any children (i.e. when someone opens DevTools), so a frame
isn't needed normally.

Comment 1

[Mats Palmgren (inactive)]

I should also mention that devtools actually fails to remove the anon content
it added, so if you open/close DevTools on the same page a few times, the
number of children in the document's custom content container grows.

Comment 2

[Mats Palmgren (inactive)]

Attachment: part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

This fixes the problem described in comment 1.

Comment 3

[Mats Palmgren (inactive)]

Attachment: part 2 - Don't create a frame for the custom content container when it has no children.

Create/destroy the frame on demand by toggling @hidden.

Comment 4

[Mats Palmgren (inactive)]

Attachment: part 3 - Use GetParentOrPlaceholderFor (not GetParent) so that reframing anonymous content frames works also for fixed pos frames.

https://tbpl.mozilla.org/?tree=Try&rev=01359209c378

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8542890 [details] [diff] [review]
part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

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

LGTM. Good catch. Thanks!

Comment 6

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/652bd77f36c1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e51dc838a89
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2d510fbd62

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for being the most-likely culprit in the push for making Linux opt mochitest-dt permafail.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f32089c6733

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5112839&repo=mozilla-inbound

Comment 8

[Mats Palmgren (inactive)]

Sorry about that, Ryan.  It's definitely surprising that this change caused a failure
in Opt builds only.  Investigating...

Comment 9

[Mats Palmgren (inactive)]

OK, so part 1 by itself fails with the same error:
https://tbpl.mozilla.org/?tree=Try&rev=0aff472a11e3

It looks like an existing bug in RemoveAnonymousContent:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=cfccc5448963#5297
(the return type of Length() is size_t so when it is zero ... oops)

Comment 10

[Mats Palmgren (inactive)]

Attachment: part 0 - Iterate forward instead, to avoid depending on undefined integer behavior.

None of these loops needs to iterate backwards.  Only the first modifies
the array and it explicitly breaks out of the loop after that.

This patch doesn't fix the failed test unfortunately, but is seems worth
taking anyway to simplify the code.

Comment 11

[Mats Palmgren (inactive)]

With part 1 applied (so we actually get a call to nsIDocument::RemoveAnonymousContent)
the test browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js
times out.  But only in Opt builds.  I can't reproduce that locally unfortunately,
neither in Debug nor Opt builds (on Linux64 and OSX).  I'm guessing it's timing
dependent.  So far, I've concluded that it is the RemoveElementAt(i) here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp?rev=00996e98fa62#5313
that makes the test time out.  Putting in a 'return;' after the RemoveElementAt
call fails consistently: https://tbpl.mozilla.org/?tree=Try&rev=0be7353043b2
Putting a return before the RemoveElementAt call makes the tests pass (but then
RemoveAnonymousContent isn't really doing anything of course).

All this smells kind of fishy to me.  I don't see why removing an entry from an
array should cause a test failure like that.  And it's not the removing as such;
dropping of the ref count is enough -- I tried two variations of that, one that
nulled out the AnonymousContent* in the array entry, and one version that nulled
out the mContentNode inside the AnonymousContent itself - both fails in the
same way.

Can anyone else reproduce it locally (in an Opt build, with part 1 applied)?
(I'm guessing a slow-ish machine has a better chance making the test fail)
The command is:
./mach mochitest-devtools browser/devtools/webconsole/test/

Comment 12

[Mats Palmgren (inactive)]

Attachment: Log snippet for posterity

I think the first "this.docShell is null" error is a red herring -
it occurs also when the test succeeds, so all is well up to the
"promise.all resolved. start testing the Browser Console" message.
http://hg.mozilla.org/mozilla-central/annotate/84880f27bab3/browser/devtools/webconsole/test/browser_bug_871156_ctrlw_close_tab.js
The "error occurred while processing 'getCachedMessages:..." that
follows probably has something to do with the timeout - I don't
see that error in a test run that succeeds.

Comment 13

[Mats Palmgren (inactive)]

Attachment: Log snippet #2

FWIW, when I comment out the latter part of browser_bug_871156_ctrlw_close_tab.js
the failure occurs in the second test after it instead: browser_console.js
https://tbpl.mozilla.org/?tree=Try&rev=fdea70e13ed2

The error messages appears to be the same as in the unmodified
browser_bug_871156_ctrlw_close_tab.js case so I'm starting to suspect
that this is an existing bug in the devtools code.  It seems that by
not releasing the anonymous content extends the lifetime of something,
which hides that bug.

Comment 14

[Mats Palmgren (inactive)]

Landed part 0:
https://hg.mozilla.org/integration/mozilla-inbound/rev/324793f2a92b

Comment 15

[Mats Palmgren (inactive)]

I'm stuck here on the devtools test failures which I don't know how to fix.
Patrick, is that something you can help on?  or know someone on the devtools
team who can?

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/324793f2a92b

Comment 17

[Patrick Brosset <:pbro>]

(In reply to Mats Palmgren (:mats) from comment #15)
> I'm stuck here on the devtools test failures which I don't know how to fix.
> Patrick, is that something you can help on?  or know someone on the devtools
> team who can?
Sorry for the delay getting involved here. I'll start taking a look at the devtools test failures today.

Comment 18

[Patrick Brosset <:pbro>]

I can't reproduce this issue either (after applying the part 1 patch, OPT build, OSX).

The weird thing is that during the webconsole tests suite execution, there's no call to removeAnonymousContent before the browser_console_native_getters.js test, which runs a lot later than browser_bug_871156_ctrlw_close_tab.js, so I don't see how a change in this function could impact this test.

Also, worth noting the test only fails on Linux, Opt, non-e10s, it seems to pass fine on other platforms, and when e10s is ON.

I agree that the "getCachedMessages" error message is related to the timeout, but it's odd because it's a message for an error that occurs on the actor side, when retrieving messages for the console, so it has nothing to do with inserting/removing anonymous content into the canvas frame, this is done at a later stage, when console messages are sent back to the UI and contain DOM nodes.

The stacktrace shows that the error occurs in DevToolsUtils.js:301, when calling |Cu.getGlobalForObject(aObj)|, which is something called while processing the |getCachedMessages| request.

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #18)
> The stacktrace shows that the error occurs in DevToolsUtils.js:301, when
> calling |Cu.getGlobalForObject(aObj)|, which is something called while
> processing the |getCachedMessages| request.
Also worth noting that |Cu.getGlobalForObject(aObj)| isn't called during browser_bug_871156_ctrlw_close_tab.js, so the error is probably from an earlier test, being reported late.

Comment 20

[Patrick Brosset <:pbro>]

Hmmm, I pushed to try (dt,linux,opt,non-e10s) with part 1 applied and a debug patch with various logs here and there for me to understand the problem better and it came back green :|
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a824ead7249c
What am I missing?

Comment 21

[Mats Palmgren (inactive)]

Attachment: part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

Hmm, I don't know why that makes it green, but I'm totally going to
exploit that as a wallpaper so I can land my changes.

I'll file a follow-up bug for the devtools test failures without the
Cu.getGlobalForObject thing.

Comment 22

[Mats Palmgren (inactive)]

Try run with all the patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4514a471f45c

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8550807 [details] [diff] [review]
part 1 - Store the document where we added anonomous content so we can call removeAnonymousContent() on it later without throwing.

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +421,5 @@
>  function CanvasFrameAnonymousContentHelper(tabActor, nodeBuilder) {
>    this.tabActor = tabActor;
>    this.nodeBuilder = nodeBuilder;
> +  this.anonymousContentDocument = this.tabActor.window.document;
> +  this.anonymousContentGlobal = Cu.getGlobalForObject(this.anonymousContentDocument);

So if I understand correctly, you're saying what makes the test green is adding this line in? Even though this.anonymousContentGlobal isn't used anywhere?
If yes, then definitely a follow-up bug is needed, but please also add a comment before this line explaining why this is here, and mentioning the bug number.

Comment 24

[Mats Palmgren (inactive)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #23)
> So if I understand correctly, you're saying what makes the test green is
> adding this line in? Even though this.anonymousContentGlobal isn't used
> anywhere?

Yes, just calling getGlobalForObject and keeping the reference seems to be enough.

> If yes, then definitely a follow-up bug is needed, but please also add a
> comment before this line explaining why this is here, and mentioning the bug
> number.

Done, bug 1123362.

Comment 25

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0a19bda7d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc3300cb6d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/700835de77fe

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ba0a19bda7d5
https://hg.mozilla.org/mozilla-central/rev/1bc3300cb6d1
https://hg.mozilla.org/mozilla-central/rev/700835de77fe

Comment 27

[Oleksandr Shybystyi]

Note that https://hg.mozilla.org/mozilla-central/rev/700835de77fe adds ability to style anonymous canvas frame.

I used the next workaround for ff37:

> /** 
>  * Manually trigger reflow for document because we can't do that for anonymouse element
>  * https://developer.mozilla.org/en-US/docs/Tools/Web_Console#synchronous-and-asynchronous-reflows
>  * this is not necessary in ff38 because of https://hg.mozilla.org/mozilla-central/rev/700835de77fe
>  */
> function windowContentReflow(win) {
>   let contentWin = win.content.window,
>       oldDisplay = contentWin.document.documentElement.style.display;
> 
>   contentWin.document.documentElement.style.display = 'none';
>   contentWin.getComputedStyle(contentWin.document.documentElement).display; // force reflow
>   contentWin.document.documentElement.style.display = oldDisplay;
>   contentWin.getComputedStyle(contentWin.document.documentElement).display;
> }

You can see the working example here https://github.com/firebug/pixel-perfect/pull/66/files