Closed Bug 1323861 Opened 8 years ago Closed 7 years ago

Reader Mode displays "Failed to load article from page" on all Breitbart articles

Categories

(Toolkit :: Reader Mode, defect)

Product:
Toolkit
Component:
Reader Mode
Version:
53 Branch
Platform:
x86_64
Windows 10
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: smartfon.reddit, Assigned: evanxd)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

firefox reader more error.png
9.38 KB, image/png
Details
Bug 1323861, Bug 1322674, Bug 1217007 - Update readability from github repo,
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20161215061212

Steps to reproduce:

Open any Breitbart article after 2016-12-13 (+-2 days)


Actual results:

Reader Mode displays a horizontal line that would normally indicate a beginning or end of a paragraph. Below the line it says "Failed to load article from page". The rest of the screen is blank. No vertical menu panel on the right.


Expected results:

It should load the article and display the vertical menu panel on the right.
Component: Untriaged → Reader Mode
Product: Firefox → Toolkit

    
  
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64

    
    

    
  
I can reproduce the issue on http://www.breitbart.com/tech/2016/12/22/neutral-snopes-fact-checker-david-emery-un-angry-trump-supporters/ .
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
  
Blocks: 1324630

    
    

    
  
I think this can also fix the Issue 334 on GitHub Issues.

[1]: https://github.com/mozilla/readability/issues/334

    
    

    
  
Found the root cause, which seems to be the JSDOMParser issue, and added tests[1].

When I run the test for the page[2], it will cause the below error.
```
JSDOMParser error: expected '</head>' and got </scrip

JSDOMParser error: expected '</html>' and got </scrip

Parsing this DOM caused errors: expected '</head>' and got </scrip
expected '</html>' and got </scrip

        1) "before all" hook


  6 passing (2s)
  1 failing

  1) Test pages breitbart JSDOMParser "before all" hook:
     TypeError: Cannot read property 'documentElement' of null
      at Readability.parse (Readability.js:1878:25)
      at Context.<anonymous> (test/test-readability.js:65:27)
```

[1]: https://github.com/evanxd/readability/commit/26c9d349292e03cc582e9de3a4a099416a8189a6
[2]: http://www.breitbart.com/tech/2016/12/22/neutral-snopes-fact-checker-david-emery-un-angry-trump-supporters/

    
  
Assignee: nobody → evan
Status: NEW → ASSIGNED

    
    

    
  
Fixed test failures[1] and discussed the solution with reviewer.

[1]: https://github.com/mozilla/readability/pull/345/commits/4d6a5b168d82c751284913c7417ec89822f540cf

    
    

    
  
Wrote a patch[1] to fix #334 and tests. And the CI is good. It's on review process...

[1]: https://github.com/mozilla/readability/pull/345/commits/ec5c19c5be3afca48fe98f0b1628f461a8fb8f29

    
    

    
  
Updated the patch for review comments and fixed all jsdomparser tests[1].

[1]: https://github.com/mozilla/readability/pull/345/commits/e53afded5efa16ec9bce946e73c8ebb43ed6dfe8

    
    

    
  
Continue to fix the rest of readability test failures[1].

[1]: https://travis-ci.org/mozilla/readability/jobs/200230979

    
    

    
  
Fixed testing failures by replacing `<` with &lt; in an embedded script without CDATA[1] except `bbc-1`, `buzzfeed-1`, `herald-sun-1`, and `liberation-1` tests. Continue to work on the rest of failures tomorrow...

[1]https://github.com/mozilla/readability/pull/345/commits/66c7f42a2700179b48db7665e4a9eb1b5a6a1d89

    
    

    
  
Updated the patch to replace `<` with `&lt;` to pass all tests: https://github.com/mozilla/readability/pull/345/commits/ea166e07ef09d2e133e51e09b6e3b5708151bf9c

Really took a lot of time to do that...

    
    

    
      
  


  

    
    

    
  
Hi Gijs,

Let's update readability from github repo. Could you take a look? Thanks.

    
    

    
  
Ensure the try[1] is good(it is running) before we land it.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7583215f6c

    
    

    
  
Comment on attachment 8840334 [details]
Bug 1323861, Bug 1322674, Bug 1217007 - Update readability from github repo,

https://reviewboard.mozilla.org/r/114842/#review116328

        Attachment #8840334 -
        Flags: review?(gijskruitbosch+bugs) → review+

    
    

    
  
Hi Wes,

Could you help land the patch(Comment 20) into m-c? Thanks.
Flags: needinfo?(wkocher)

    
    

    
  
I'm already out for the night. Tomcat has a better chance of getting this before I do.
Flags: needinfo?(wkocher) → needinfo?(cbook)

    
    

    
  
Thanks for helping, Wes.

    
    

    
  
(In reply to Pulsebot from comment #20)
> Pushed by gijskruitbosch@gmail.com:
> https://hg.mozilla.org/integration/autoland/rev/e04079f3f386
> Bug 1322674, Bug 1217007 - Update readability from github repo, r=Gijs

This has already landed on m-c but wasn't marked for whatever reason.

https://hg.mozilla.org/mozilla-central/rev/e04079f3f386
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

    
  
Flags: needinfo?(cbook)

    
    

    
  
(In reply to :Gijs from comment #24)
> (In reply to Pulsebot from comment #20)
> > Pushed by gijskruitbosch@gmail.com:
> > https://hg.mozilla.org/integration/autoland/rev/e04079f3f386
> > Bug 1322674, Bug 1217007 - Update readability from github repo, r=Gijs
> 
> This has already landed on m-c but wasn't marked for whatever reason.
> 
> https://hg.mozilla.org/mozilla-central/rev/e04079f3f386

because of https://bugzilla.mozilla.org/show_bug.cgi?id=1115608#c7 - the autoland push is currently not displayed on treeherder so wasn't able to mark it :( sorry

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: