993190 - Remote inspector makes page content disappear

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[:Margaret Leibovic]

I'm trying to debug about:addons on Fennec, and I found that all the way up to beta the page content will disappear as soon as I try to use the remote inspector.

I also found that this is a problem on regular websites.

I'm using desktop Nightly, but I feel like the issue must be on the Fennec side, since this isn't a problem if I try using Fennec release.

Comment 1

[:Margaret Leibovic]

I'm going to move this over to Firefox for Android, because I feel like this might be some sort of gfx/layout issue on our end.

To clarify, the DOM does not go away, and the inspector still appears to work on desktop, but the page itself goes white, with some flashing of text or black boxes at times.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This was a tricky one to bisect because you need to have at least 2 tabs open before the issue will reproduce.

Last good revision: b02827c3d07d
First bad revision: 5fc335a9b782
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b02827c3d07d&tochange=5fc335a9b782

There are still many changes in that list, but this one seems highly suspect:

	c3145720f680	Patrick Brosset — Bug 916443 - part 2 - Toolbox level remote highlighter (tests), r=paul
	bc5a874afdf7	Patrick Brosset — Bug 916443 - part 1 - Toolbox level remote highlighter [Australis], r=paul

Comment 3

[Patrick Brosset <:pbro>]

So when Bug 916443 landed, the remote highlighter definitely used to work on Fennec.
Right not, it somehow completely covers the page.

As Margaret said, the DOM is still here and the devtools do work (you can for instance change styles, etc...), the only thing is that the highlighter seems to be creating a 100% width/height white element on top of the content so you don't see anything.

What you do see though while moving the mouse over elements in the inspector is the element identifier <element#id.class> that we normally display in the highlighter node info-bar.

This node info-bar normally only exists for desktop targets. Indeed we really have 2 highlighters, one simple (just a red dashed outline) that we use for fxos and fennec because those two do not offer a parent browser node we can use to append a more complex highlighter. And one more complex one, based on XUL (and SVG today) which displays boxes on top of the element, and a node info-bar.

This problem definitely doesn't occur with Firefox 28 (before we moved to the remote highlighter), so Bug 916443 did start this, but again this used to work right when it landed.

Here is the test we run to use the simple or complex highlighter:

  /**
   * Can the host support the box model highlighter which requires a parent
   * XUL node to attach itself.
   */
  _supportsBoxModelHighlighter: function() {
    return this._tabActor.browser && !!this._tabActor.browser.parentNode;
  },

Did Fennec recently change in a way that this would now return true?
For info, this._tabActor.browser here is one item of the gBrowser.browsers array.

If this now returns true on Fennec, it's good in a way because it means we might be able to use the nicer box-model highlighter we have on desktop, but seeing that the page is covered by a 100% width/height white element probably means that the highlighter CSS is missing completely.

Being able to see the logs from the phone would help. How can I do that?

Comment 4

[:Margaret Leibovic]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)

> Here is the test we run to use the simple or complex highlighter:
> 
>   /**
>    * Can the host support the box model highlighter which requires a parent
>    * XUL node to attach itself.
>    */
>   _supportsBoxModelHighlighter: function() {
>     return this._tabActor.browser && !!this._tabActor.browser.parentNode;
>   },
> 
> Did Fennec recently change in a way that this would now return true?
> For info, this._tabActor.browser here is one item of the gBrowser.browsers
> array.

I'm confused about how this would have ever returned false... the browser elements that we display in tabs are all children of a XUL deck, and it's been that way since our native fennec rewrite (so, years).

> If this now returns true on Fennec, it's good in a way because it means we
> might be able to use the nicer box-model highlighter we have on desktop, but
> seeing that the page is covered by a 100% width/height white element
> probably means that the highlighter CSS is missing completely.

This could be interesting to explore, although I don't know that it would work because we have special logic for drawing the content of our XUL browser elements, and our main XUL window is more of a container that we can hook into, rather than a thing that is drawn to the screen.

> Being able to see the logs from the phone would help. How can I do that?

You should be able to use the remote debugger to see browser console output (if you attach to "Main Process"). You can also use adb logcat to get all of the logging from the phone.

Comment 5

[Patrick Brosset <:pbro>]

So if you debug the main process, then the expected red outline is used for the highlighter.
The problem only occurs if you debug a tab and, as Margaret said, since the tab browser is a children of the XUL deck, the highlighter uses the newer box-model structure, which doesn't work very well.
On about:addons it covers the whole page, while on other pages it's just not visible at all.

I need a way to detect, from the devtools server-side code, whether the host is Fennec or not, and use this to always use the outline highlighter.

Comment 6

[Patrick Brosset <:pbro>]

Margaret, so far the only way I found how to detect the app type is by checking the type of browser.parentNode which, on Fennec, is a <deck> and on Desktop is a <stack>. Do you know of any other way I could detect this?

The other thing is, I've never built Fennec locally and deployed to a device. Is there a quick way I could test this fix?
Something like this would work I guess:

  _supportsBoxModelHighlighter: function() {
    // Note that <browser>s on Fennec also have a XUL parentNode, and there's no
    // way to detect we're on Desktop rather than Fennec other than checking if
    // the parentNode is a <stack>
    return this._tabActor.browser &&
           !!this._tabActor.browser.parentNode &&
           this._tabActor.browser.parentNode.tagName === "STACK";
  },

Comment 7

[Patrick Brosset <:pbro>]

Or rather something like ...parentNode.localName === "stack"

Comment 8

[Patrick Brosset <:pbro>]

Perhaps importing Utils.jsm and using the MozBuildApp would be better.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

How about Services.appInfo.name?

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/device.js#85

Comment 10

[:Margaret Leibovic]

(In reply to Panos Astithas [:past] from comment #9)
> How about Services.appInfo.name?
> 
> http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/
> device.js#85

I like this idea, since this feels like a less fragile way to check the app than inspecting the XUL DOM. I just tested, and desktop returns "Firefox", whereas Android returns "Fennec" (I double-checked on nightly and release to make sure this wasn't affected by branding). 

Building fennec locally isn't too hard, you basically just need to download some Android/Java dependencies and swap in a different mozconfig (you can have separate object directories, which is handy for building desktop/mobile in the same directory):
https://wiki.mozilla.org/Mobile/Fennec/Android

But I can also just test a patch for you myself, since setting up a build environment is probably too much work for this one bug :)

Comment 11

[Patrick Brosset <:pbro>]

Attachment: bug993190-remote-highlighter-fennec v1.patch

Thanks Panos, that does seem like the best solution.
Margaret, since you mention it :) it would really be super useful if you could try this patch on Fennec.

Comment 12

[:Margaret Leibovic]

Comment on attachment 8405425 [details] [diff] [review]
bug993190-remote-highlighter-fennec v1.patch

Yay, that works!

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8405425 [details] [diff] [review]
bug993190-remote-highlighter-fennec v1.patch

Thanks for testing Margaret!
Mike, a quick review on that one?

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8405425 [details] [diff] [review]
bug993190-remote-highlighter-fennec v1.patch

Minor drive-by. Could we use:

Services.appinfo.ID !== "{a23983c0-fd0e-11dc-95ff-0800200c9a66}";

?

We use similar code in devtools for b2g:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#754
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webapps.js#845

Comment 15

[Sylvestre Ledru [:Sylvestre]]

To make sure this goes in 29, it would be nice to have an uplift done today (Monday) before the 29 gtb (around 12 PDT)

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8405425 [details] [diff] [review]
bug993190-remote-highlighter-fennec v1.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 916443

User impact if declined: If users want to inspect web pages remotely on fennec with the devtools, the highlighter won't be visible at all. If users want to inspect the "about:addons" page remotely on fennec with the devtools, the page will be blank.

Testing completed (on m-c, etc.): Testing completed on the device directly, by Margaret.

Risk to taking this patch (and alternatives if risky): The risk is very limited. The line that changed is an extra condition in the list of conditions we use to decide whether the device/browser supports the new highlighter or not.

String or IDL/UUID changes made by this patch: None

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Patrick, maybe this is coming but you haven't requested an uplift to beta, is that expected?

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8405425 [details] [diff] [review]
bug993190-remote-highlighter-fennec v1.patch

[Approval Request Comment]
Exact same approval request comment as for aurora. This code hasn't changed between beta and aurora anyway.

Comment 19

[Patrick Brosset <:pbro>]

I missed mfinkle's comment, I will update the patch to rely on the ID instead.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: bug993190-remote-highlighter-fennec v2.patch

Just changed the condition from using the "fennec" app name to the actual "mobile/xul" ID, to be more future-proof.

[Approval Request Comment]
Requesting approval to Aurora/Beta again, knowing that the comment is the same as when I requested approval earlier today.

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> Just changed the condition from using the "fennec" app name to the actual
> "mobile/xul" ID, to be more future-proof.

Wait, mfinkle did you actually mean to suggest {aa3c5121-dab2-40e2-81ca-7ea25febc110} instead?

Comment 22

[:Margaret Leibovic]

(In reply to Panos Astithas [:past] from comment #21)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> > Just changed the condition from using the "fennec" app name to the actual
> > "mobile/xul" ID, to be more future-proof.
> 
> Wait, mfinkle did you actually mean to suggest
> {aa3c5121-dab2-40e2-81ca-7ea25febc110} instead?

I think he did. On Fennec, Services.appinfo.ID returns "{aa3c5121-dab2-40e2-81ca-7ea25febc110}".

Comment 23

[:Margaret Leibovic]

We should update the patch here to use the correct app ID and get it landed. Firefox 29 is getting close to release :)

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to :Margaret Leibovic from comment #22)
> (In reply to Panos Astithas [:past] from comment #21)
> > (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #20)
> > > Just changed the condition from using the "fennec" app name to the actual
> > > "mobile/xul" ID, to be more future-proof.
> > 
> > Wait, mfinkle did you actually mean to suggest
> > {aa3c5121-dab2-40e2-81ca-7ea25febc110} instead?
> 
> I think he did. On Fennec, Services.appinfo.ID returns
> "{aa3c5121-dab2-40e2-81ca-7ea25febc110}".

Thanks for catching this. Copy/Paste strikes again.

Comment 25

[Patrick Brosset <:pbro>]

Attachment: bug993190-remote-highlighter-fennec v3.patch

Sorry about the delay, here is a new patch with the correct appID for Fennec.
Request aurora/beta uplift approval for this patch too! Sorry, didn't realize the appID was the wrong one. Approval request comment is the same as the original one.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8407359 [details] [diff] [review]
bug993190-remote-highlighter-fennec v3.patch

Approving because if the feature can be unusable and because the patch is minor and should be indeed low risk.

Comment 27

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/8e92a7640d74

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e92a7640d74

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/770e11b422e3
https://hg.mozilla.org/releases/mozilla-beta/rev/ddbb75065940