Skip to content

/ servo

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

Make ServoParser::pending_input hold onto a BufferQueue #14250

Merged
merged 1 commit into from Nov 19, 2016
Merged

Make ServoParser::pending_input hold onto a BufferQueue #14250

merged 1 commit into from Nov 19, 2016

Conversation

@nox
Copy link
Member

nox commented Nov 16, 2016 •
edited by larsbergstrom

This change is Reviewable
@SimonSapin
Copy link
Member

SimonSapin commented Nov 16, 2016

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

components/script/dom/servoparser/xml.rs, line 54 at r1 (raw file):

    pub fn feed(&mut self, input: &mut BufferQueue) {
        if let Some(chunk) = input.pop_front() {

Should this be a loop? r=me otherwise

Comments from Reviewable
@nox nox force-pushed the nox:write branch from cc8de1d to 994c2c7 Nov 16, 2016
@nox nox changed the title Make ServoParser::pending_input hold onto a BufferQueue (Do not merge) Make ServoParser::pending_input hold onto a BufferQueue Nov 16, 2016
@bors-servo
Copy link
Contributor

bors-servo commented Nov 16, 2016

☔ The latest upstream changes (presumably #14172) made this pull request unmergeable. Please resolve the merge conflicts.
@nox nox force-pushed the nox:write branch 2 times, most recently from f5c8150 to bdd2169 Nov 17, 2016
@nox
Copy link
Member Author

nox commented Nov 17, 2016

@bors-servo try
@bors-servo
Copy link
Contributor

bors-servo commented Nov 17, 2016

⌛ Trying commit bdd2169 with merge 957b1c9...
bors-servo added a commit that referenced this pull request Nov 17, 2016
@bors-servo
Auto merge of #14250 - nox:write, r=<try>
Loading status checks…
957b1c9 
(Do not merge) Make ServoParser::pending_input hold onto a BufferQueue

<!-- 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/14250)
<!-- Reviewable:end -->
@nox
nox reviewed Nov 17, 2016
View changes
components/script/dom/servoparser/xml.rs Outdated
};
}
NextParserState::Continue
self.script.set(Some(node.downcast().unwrap()));

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Nov 17, 2016

Author Member

This is the only way I found to make xml5ever just stop tokenizing on </script> to handle the script itself out of it.
@nox
nox reviewed Nov 17, 2016
View changes
components/script/dom/servoparser/mod.rs
self.tokenizer.borrow_mut().run();
if let Err(script) = self.tokenizer.borrow_mut().feed(&mut *self.pending_input.borrow_mut()) {
if script.prepare() {
continue;

This comment has been minimized.

Sign in to view
Copy link
@nox

nox Nov 17, 2016

Author Member

In the case where tokenization needs to continue after a </script>, we must preemptively continue because xml5ever might be holding onto unprocessed input in the tokenizer itself, even if input is empty.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 17, 2016

💔 Test failed - mac-rel-wpt1
@bors-servo
Copy link
Contributor

bors-servo commented Nov 17, 2016

☔ The latest upstream changes (presumably #14246) made this pull request unmergeable. Please resolve the merge conflicts.
@nox nox force-pushed the nox:write branch from bdd2169 to bb2b554 Nov 18, 2016
@nox nox force-pushed the nox:write branch from bb2b554 to 3041e1c Nov 18, 2016
@nox
Copy link
Member Author

nox commented Nov 18, 2016 

  ▶ CRASH [expected TIMEOUT] /html/semantics/scripting-1/the-script-element/script-text-xhtml.xhtml
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ called `Option::unwrap()` on a `None` value (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }, at ../src/libcore/option.rs:323)
  │ stack backtrace:
  │    0:        0x10e6df1de - backtrace::backtrace::trace::h16372ee7bf1517e5
  │    1:        0x10e6df4cc - backtrace::capture::Backtrace::new::hcc43c50c4b11c693
  │    2:        0x10d571e44 - servo::main::_{{closure}}::he01100fc53982b46
  │    3:        0x10ef8d923 - std::panicking::rust_panic_with_hook::h00b81bb3dcbd51f2
  │    4:        0x10ef8d7f4 - std::panicking::begin_panic::ha6a0d553db9869ff
  │    5:        0x10ef8d712 - std::panicking::begin_panic_fmt::h24d113aee3ee4081
  │    6:        0x10ef8d677 - rust_begin_unwind
  │    7:        0x10efba440 - core::panicking::panic_fmt::he441b2ea2036b98a
  │    8:        0x10efba344 - core::panicking::panic::haf296e94ad32f436
  │    9:        0x10dc5125b - _<script..dom..servoparser..xml..Sink as xml5ever..tree_builder..interface..TreeSink>::complete_script::h363a22d4574fe967
  │   10:        0x10d7a8305 - _<xml5ever..tree_builder..XmlTreeBuilder<Handle, Sink> as xml5ever..tokenizer..interface..TokenSink>::process_token::h5352793fa63d0833
  │   11:        0x10da2414d - _<xml5ever..tokenizer..XmlTokenizer<Sink>>::process_token::h298bec2821ee1492
  │   12:        0x10da246ef - _<xml5ever..tokenizer..XmlTokenizer<Sink>>::emit_current_tag::hca225c6642ecfea1
  │   13:        0x10da2739d - _<xml5ever..tokenizer..XmlTokenizer<Sink>>::step::h8baab31b871abdc7
  │   14:        0x10da259e7 - _<xml5ever..tokenizer..XmlTokenizer<Sink>>::run::h87e3b4d6a1824607
  │   15:        0x10dc53f2e - script::dom::servoparser::ServoParser::parse_sync::h6edc19833d00e6a4
  │   16:        0x10db1c6e5 - script::dom::document::Document::finish_load::hf165040e1edb94ab
  │   17:        0x10dbd449b - _<script..dom..htmlscriptelement..ScriptContext as net_traits..FetchResponseListener>::process_response_eof::h86f91cedb9e1b62a
  │   18:        0x10dcd4b5a - _<script..network_listener..ListenerRunnable<A, Listener> as script..script_thread..Runnable>::handler::h63993bd2bbce91fa
  │   19:        0x10dcdf472 - _<script..script_thread..CancellableRunnable<T> as script..script_thread..Runnable>::handler::h74f55f00d39311c3
  │   20:        0x10dcf8451 - script::script_thread::ScriptThread::handle_msg_from_script::h4a8186d0d2190624
  │   21:        0x10dea612c - script::script_thread::ScriptThread::handle_msgs::_{{closure}}::heac91dc058b267c7
  │   22:        0x10dceef6c - script::script_thread::ScriptThread::handle_msgs::hef5c3f0cf849f64e
  │   23:        0x10d846e37 - std::panicking::try::do_call::h273505ea6976b345
  │   24:        0x10ef8e94a - __rust_maybe_catch_panic
  │   25:        0x10da0f936 - _<F as alloc..boxed..FnBox<A>>::call_box::hfed4a82f33ee708a
  │   26:        0x10ef8c7d4 - std::sys::thread::Thread::new::thread_start::h9fa66f6812e81e10
  │   27:     0x7fff89039059 - _pthread_body
  │   28:     0x7fff89038fd6 - _pthread_start
  │ ERROR:servo: called `Option::unwrap()` on a `None` value
  └ Pipeline failed in hard-fail mode.  Crashing!

So apparently xml5ever sometimes calls TreeSink::complete_script with nodes that aren't scripts, great...
@nox
Make ServoParser::pending_input hold onto a BufferQueue
Loading status checks…
a3da819
@nox nox force-pushed the nox:write branch from 3041e1c to a3da819 Nov 18, 2016
@nox
Copy link
Member Author

nox commented Nov 18, 2016

@bors-servo try
bors-servo added a commit that referenced this pull request Nov 18, 2016
@bors-servo
Auto merge of #14250 - nox:write, r=<try>
Loading status checks…
a90f936 
(Do not merge) Make ServoParser::pending_input hold onto a BufferQueue

<!-- 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/14250)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

⌛ Trying commit a3da819 with merge a90f936...
@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

💔 Test failed - linux-rel-wpt
@highfive
Copy link

highfive commented Nov 18, 2016 

  ▶ FAIL [expected PASS] /_mozilla/css/incremental_trailing_whitespace_a.html
  └   → /_mozilla/css/incremental_trailing_whitespace_a.html da54041086f2975f8e3c776d2283ad6609e6862a
/_mozilla/css/incremental_trailing_whitespace_ref.html f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
Testing da54041086f2975f8e3c776d2283ad6609e6862a == f5d0147c8bddbdc772a7e5e86c7c9e433fcd486b
@nox nox changed the title (Do not merge) Make ServoParser::pending_input hold onto a BufferQueue Make ServoParser::pending_input hold onto a BufferQueue Nov 18, 2016
@nox
Copy link
Member Author

nox commented Nov 18, 2016

r? @SimonSapin
@SimonSapin
Copy link
Member

SimonSapin commented Nov 18, 2016

@bors-servo r+

Reviewed 1 of 7 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

📌 Commit a3da819 has been approved by SimonSapin
@nox
Copy link
Member Author

nox commented Nov 19, 2016

@bors-servo retry #10473
@bors-servo
Copy link
Contributor

bors-servo commented Nov 19, 2016

⌛ Testing commit a3da819 with merge fdcf510...
bors-servo added a commit that referenced this pull request Nov 19, 2016
@bors-servo
Auto merge of #14250 - nox:write, r=SimonSapin
Loading status checks…
fdcf510 
Make ServoParser::pending_input hold onto a BufferQueue

<!-- 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/14250)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 19, 2016

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev
@bors-servo bors-servo merged commit a3da819 into servo:master Nov 19, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@SimonSapin SimonSapin

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@nox @SimonSapin @bors-servo @highfive
You can’t perform that action at this time.