Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User scripts don't apply correctly in about:blank #15082

Closed
jdm opened this issue Jan 17, 2017 · 15 comments
Closed

User scripts don't apply correctly in about:blank #15082

jdm opened this issue Jan 17, 2017 · 15 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 17, 2017

Our implementation of user scripts appends <script src='file://...'></script> to the <head> element in every document is parsed. This breaks the synchronous nature of about:blank, since that's an asynchronous network request that interrupts parsing. I have a patch that embeds the user script source as an inline script instead.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 17, 2017

diff --git a/components/script/dom/userscripts.rs b/components/script/dom/userscripts.rs
index 66758af..16a1b61 100644
--- a/components/script/dom/userscripts.rs
+++ b/components/script/dom/userscripts.rs
@@ -5,14 +5,15 @@
 use dom::bindings::codegen::Bindings::DocumentBinding::DocumentMethods;
 use dom::bindings::codegen::Bindings::NodeBinding::NodeMethods;
 use dom::bindings::inheritance::Castable;
-use dom::bindings::js::RootedReference;
+use dom::bindings::js::{RootedReference, Root};
 use dom::bindings::str::DOMString;
 use dom::htmlheadelement::HTMLHeadElement;
 use dom::node::Node;
 use servo_config::opts;
 use servo_config::resource_files::resources_dir_path;
 use std::borrow::ToOwned;
-use std::fs::read_dir;
+use std::fs::{File, read_dir};
+use std::io::Read;
 use std::path::PathBuf;
 
 
@@ -40,12 +41,18 @@ pub fn load_script(head: &HTMLHeadElement) {
         files.sort();
 
         for file in files {
-            let name = match file.into_os_string().into_string() {
+            /*let name = match file.into_os_string().into_string() {
                 Ok(ref s) if s.ends_with(".js") => "file://".to_owned() + &s[..],
                 _ => continue
-            };
+        };*/
+            //let name = file.into_os_string().into
+            let mut f = File::open(&file).unwrap();
+            let mut contents = vec![];
+            f.read_to_end(&mut contents).unwrap();
             let new_script = doc.CreateElement(DOMString::from("script")).unwrap();
-            new_script.set_string_attribute(&local_name!("src"), DOMString::from(name));
+            let script_text = Root::upcast(doc.CreateTextNode(String::from_utf8(contents).unwrap().into()));
+            new_script.upcast::<Node>().AppendChild(&*script_text).unwrap();
+            //new_script.set_string_attribute(&local_name!("src"), DOMString::from(name));
             node.InsertBefore(new_script.upcast(), first_child.r()).unwrap();
         }
     }
@jdm jdm added the C-has-patch label Jan 17, 2017
@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Jan 19, 2017

At this point, why not just call evaluate_js_script or whatever it's called?

@jdm
Copy link
Member Author

@jdm jdm commented Jan 19, 2017

That's... a good point!

@highfive
Copy link

@highfive highfive commented Feb 8, 2017

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@jdm
Copy link
Member Author

@jdm jdm commented Feb 8, 2017

The best way to test this change:
Modify resources/user-agent-js/00.example.js to contain window.foo = 'success!';, then run ./mach run test.html --userscripts, where test.html is a page like:

<script>console.log(window.foo);</script>
<iframe></iframe>
<script>console.log(document.querySelector('iframe').contentWindow.foo);</script>

The output should be:

success!
success!

but on master right now I see

undefined
undefined
@jdm
Copy link
Member Author

@jdm jdm commented Feb 8, 2017

We'll want to call GlobalScope::evaluate_js_on_global_with_result instead of appending a <script> element to the document.

@meetmangukiya
Copy link
Contributor

@meetmangukiya meetmangukiya commented Feb 12, 2017

@highfive: assign me

@highfive highfive added the C-assigned label Feb 12, 2017
@highfive
Copy link

@highfive highfive commented Feb 12, 2017

Hey @meetmangukiya! Thanks for your interest in working on this issue. It's now assigned to you!

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Feb 27, 2017

@meetmangukiya Did you start working on this?

@jdm jdm removed the C-assigned label Mar 4, 2017
@jdm
Copy link
Member Author

@jdm jdm commented Mar 4, 2017

Unassigning due to inactivity.

@dudelson
Copy link

@dudelson dudelson commented Mar 4, 2017

@highfive: assign me

@highfive
Copy link

@highfive highfive commented Mar 4, 2017

Hey @dudelson! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Mar 4, 2017
@dudelson
Copy link

@dudelson dudelson commented Mar 8, 2017

Although I would like to work on this issue, I've become too busy to complete it at this time. Please unassign me.

@sendilkumarn
Copy link
Contributor

@sendilkumarn sendilkumarn commented Mar 8, 2017

@highfive: assign me

@highfive
Copy link

@highfive highfive commented Mar 8, 2017

Hey @sendilkumarn! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Mar 8, 2017
@sendilkumarn sendilkumarn mentioned this issue Mar 8, 2017
3 of 3 tasks complete
bors-servo added a commit that referenced this issue Mar 13, 2017
apply user scripts correctly

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #15082

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15874)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.