1377523 - Convert tests within devtools/ to not rely on data: URI inheritance

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

Recap of our in person discussion:
It seems that so many devtools tests rely on data: URI inheritance. You can test by flipping the pref [0]. As identified, an easy fix (if possible) is to use srcdoc instead of src="data:". One of those spots we have identified is found here [1]. And then all the callsites of createHost() would need to be updated to strip the 'data:...' part.

[0] https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#5683
[1] https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/test/head.js#114

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Typo when assigning the bug :-(

Comment 2

[Patrick Brosset <:pbro>]

This seems, at first sight, like a really simple one that could knock off a good number of the failing tests at once.
After investigating, it seems a little more involved.

We create an iframe here: http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/devtools/client/framework/toolbox-hosts.js#69
The iframe is being appended into the current browser tab, so in XUL-land.

And the fact that it's XUL seems to be preventing the srcdoc attribute from working at all.
I mean, the attribute can be set like any other custom attribute, but setting it has no effect on the iframe content itself.

Just to verify that, I tried creating an HTML iframe instead, and it started working.
So I think we need to stop using the host.create() function in our shared head.js file here: http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/devtools/client/shared/test/head.js#110
cause that can only create XUL iframes, and I don't want to change that.

Instead, I need to rewrite this shared createHost function so it uses an HTML iframe instead.

Comment 3

[Patrick Brosset <:pbro>]

Update: I'm still working on this. I haven't been able to spend as much time as I would have liked, so it's going slow.
The majority of the tests that use createHost are easily fixed, but a few have special corner cases that make them harder.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Just back from vacation, I have some fixes in work week, most of them are just moving to seperate files, should be able to fix most of the failures in devtools/, taking this bug.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: add dummy.xul

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: fix for browser_toolbox_*

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: fix failure in sourceeditor/

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: fix devtools/client/shared/test

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: fix styleeditor/

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 6: fix in webconsole/

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

try result 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3d9382869b27a356ed6568ad8f8336ddf896043&selectedJob=113269692

Right now there're 3 failures in devtools/ left. (Hopefully)

- devtools/client/shared/test/browser_html_tooltip_arrow-01.js
- devtools/client/shared/test/browser_html_tooltip_arrow-02.js
- devtools/client/shared/test/browser_inplace-editor_autocomplete_offset.js

Hi pbro,
Can I just move the data: URI in devtools/client/shared/test/browser_inplace-editor_autocomplete_offset.js to seperate file? I am not quite sure if the test is used to test data: URI only.

Also for the failures in devtools/client/shared/test/browser_html_tooltip_arrow-01.js, can you help to check my Part 4 patch? It looks the script is never run but I am not sure what went wrong,

Thanks

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Yoshi Huang [:allstars.chh] from comment #11)
> try result 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=d3d9382869b27a356ed6568ad8f8336ddf896043&selectedJob=1
> 13269692
> 
> Right now there're 3 failures in devtools/ left. (Hopefully)
Thanks for picking this up! And for extracting all of these files.

> Can I just move the data: URI in
> devtools/client/shared/test/browser_inplace-editor_autocomplete_offset.js to
> seperate file? I am not quite sure if the test is used to test data: URI
> only.
Yes, moving this to a separate file should be fine.

> Also for the failures in
> devtools/client/shared/test/browser_html_tooltip_arrow-01.js, can you help
> to check my Part 4 patch? It looks the script is never run but I am not sure
> what went wrong,
You need to replace all the instances of
${getAnchor("top: 0; left: 0;")}
(and other similar lines)
with the corresponding XUL markup code, and get rid of the <script> tag.
So, in this case, this line should be:

<html:div class="anchor" style="width:10px; height: 10px; position: absolute;
                                background: red; top: 0; left: 0;"></html:div>

Same in file_html_tooltip_arrow-02.xul

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: fix devtools/client/shared/test

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

try result when we turn on the pref: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48fc70ec05a40f4d3e3352d1a634638420a950d7

when the pref is off.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f0219788ad797cb25015cfa03e5513e71624982

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8885160 [details] [diff] [review]
Part 1: add dummy.xul

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

You will need a toolkit peer to review these changes since they belong to /toolkit/
They look fine to me, but I can't give R+ since I'm not a toolkit peer. See https://wiki.mozilla.org/Modules/All#Toolkit

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8885161 [details] [diff] [review]
Part 2: fix for browser_toolbox_*

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

::: devtools/client/framework/test/browser_toolbox_sidebar_events.xul
@@ +1,1 @@
> +<?xml version='1.0'?>

Please add the license header right below the <?xml...> instruction

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

::: devtools/client/framework/test/browser_toolbox_sidebar_existing_tabs.xul
@@ +1,1 @@
> +<?xml version='1.0'?>

Please add the license header right below the <?xml...> instruction

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

::: devtools/client/framework/test/browser_toolbox_sidebar_toolURL.xul
@@ +1,1 @@
> +<?xml version='1.0'?>

Please add the license header right below the <?xml...> instruction

<!-- This Source Code Form is subject to the terms of the Mozilla Public
   - License, v. 2.0. If a copy of the MPL was not distributed with this
   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8885162 [details] [diff] [review]
Part 3: fix failure in sourceeditor/

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

::: devtools/client/sourceeditor/test/head.xul
@@ +1,1 @@
> +<?xml version='1.0'?>

Same comment about license header here.

Comment 18

[Patrick Brosset <:pbro>]

Comment on attachment 8885165 [details] [diff] [review]
Part 5: fix styleeditor/

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

::: devtools/client/styleeditor/test/browser.ini
@@ +3,5 @@
>  subsuite = devtools
>  support-files =
>    autocomplete.html
>    browser_styleeditor_cmd_edit.html
> +  file_highlight-selector.html

I don't think we need the file_ prefix. Other html files in this test directory seem to be simply named after what they're used for. So I would suggest naming this file simply selector-highlighter.html

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8885575 [details] [diff] [review]
Part 4: fix devtools/client/shared/test

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

One general comment is the lack of license headers in the new html and xul files (same as my comment about earlier patches).

::: devtools/client/shared/test/browser.ini
@@ +26,5 @@
> +  file_mdn-docs-01.html
> +  file_spectrum.html
> +  file_tableWidget_basic.html
> +  file_tableWidget_keyboard_interaction.xul
> +  file_tableWidget_mouse_interaction.xul

Note that the html/xul test files in this directory do not follow any naming pattern unfortunately. Here you are introducing a new pattern by using the file_ prefix.
I think having a prefix is good because it makes it easier to separate the test files from the support files in a directory, but I don't think file_ is necessarily the right one, and if you introduce a pattern, it would be good to do the same everywhere.

In other test directories, we use the doc_ prefix.
So here's what I suggest:
- rename all your new files from file_ to doc_
- rename all browser_*.html files in this directory to doc_*.html
- rename all html-*.html files in this directory to doc_*.html

The other support files in this directory are special, let's leave them unchanged.

Is that ok?

::: devtools/client/shared/test/browser_css_angle.js
@@ +3,5 @@
>  
>  /* import-globals-from head.js */
>  "use strict";
>  
> +const TEST_URI = CHROME_URL_ROOT + "browser_css_angle.js";

nit: previous to this change, we were loading the string "browser_css_angle.js" inside the iframe, now with your change, we are actually loading the content of the browser_css_angle.js file.
I don't think this impacts the test, but is also useless for the test.
I think you could remove TEST_URI completely, and just do createHost("bottom"); below, no URL parameter.

::: devtools/client/shared/test/browser_css_color.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +const TEST_URI = CHROME_URL_ROOT + "browser_css_color.js";

Same comment as above.

Comment 20

[Patrick Brosset <:pbro>]

Thanks for working on this! Glad that most (all?) tests are now fixed.
I'll just take a second look at that last patch once you've addressed my comments, and we should be good to go.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8885160 [details] [diff] [review]
Part 1: add dummy.xul

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

forward to smaug

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8885160 [details] [diff] [review]
Part 1: add dummy.xul

Could some toolkit peer review this change.
I know it is small, but I'd prefer someone else to r+

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

oh, I am a peer https://wiki.mozilla.org/Modules/Firefox

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Patrick Brosset <:pbro> from comment #19)
> - rename all browser_*.html files in this directory to doc_*.html
> - rename all html-*.html files in this directory to doc_*.html
> 
Hi Patric
Just to be sure, I didn't introduce any new browser_*.html files, do you mean I change the original ones in that directory? 
like browser_layoutHelpers.html,... and html-mdn-css-basic-testing.html, ... etc?

Thanks

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4-1: rename to doc_

Hi Patrick
This is the renaming change, can you help to review this again?

I'll merge it with Part 4 together once they got r+

Thanks

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: fix styleeditor/. v2

rename to selector-hightlighter.html by comment 18

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a790d8b9abb1
Part 1: add dummy.xul. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfb8a491544
Part 2: fix for browser_toolbox_*. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e29e8a32baf4
Part 3: fix failure in sourceeditor/. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/e836466814de
Part 4: fix devtools/client/shared/test. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09f9523c66f
Part 5: fix styleeditor/. r=pbro
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b98c3ff6ca
Part 6: fix in webconsole/. r=pbro

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a790d8b9abb1
https://hg.mozilla.org/mozilla-central/rev/cbfb8a491544
https://hg.mozilla.org/mozilla-central/rev/e29e8a32baf4
https://hg.mozilla.org/mozilla-central/rev/e836466814de
https://hg.mozilla.org/mozilla-central/rev/b09f9523c66f
https://hg.mozilla.org/mozilla-central/rev/50b98c3ff6ca