Skip to content

Loading…

bug report: uBlock doesn't work on XHTML pages (application/xhtml+xml) #1528

Closed
gdh1995 opened this Issue · 17 comments

2 participants

@gdh1995

If a server renders a page with Content-Type: application/xhtml+xml in its header, then uBlock will give up to block anything.

This bug was imported by commit de9fad5.

if ( document instanceof HTMLDocument === false ) {
    return false;
}

Solution:
In fact, only <xml> body will cause crash in #464, and its distinctive feature is:

document.createElement('div').style is null. Here's another discussions about this crash:
philc/vimium#1640 (comment)

@gdh1995

Add: It seems better to check document.documentElement.tagName.toLowerCase() === "html".

@gorhill

Browser version, uBlock version, URL, filter lists, settings, etc.?

See CONTRIBUTING.

@gdh1995

This is a WAP page with some advertise links (their font color is red).

@gdh1995

I want tieba.baidu.com##a.advertise,a.post_client_down to block those red links. Here are screen snapshots:

Origin page, and also after blocking by uBlock Origin:
before

After blocking by my custom uBlock Origin (using documentElement.tagName.*** === "html"):
after

Edit: Oops, the page url is not what I mentioned above, but "http://tieba.baidu.com/f?kw=%E9%9A%90%E6%9D%80". But I promise that they means the same page on "tieba.baidu.com".

@gorhill

I can't see any red text like in your screenshots when I set uBlock as per your settings. for the link you provided:

a

When I load the test case for #464, the condition document instanceof HTMLDocument is false, meaning uBlock will abort cosmetic filtering. However with your proposed solution, document.documentElement.localName === 'html' is true, which means issue #464 would be back, i.e. a regression.

Anyways, I just realized I am not dealing with an issue opened on my branch...

@gdh1995

What you see is just a page for PC, but I'm talking about a page designed for mobile devices (e.g. a Android browser). So please test on http://tieba.baidu.com/mo/q---2799060AC2198378C599200012D229ED%3AFG%3D1--1-3-0--2--wapp_1440757687244_809/m?kw=%E9%9A%90%E6%9D%80&lp=1030.

And, I'm sorry but this bug is found on your "uBlock Origin".

@gdh1995

Please forgive me for that I didn't explain my solution well.

The best way to detect XML view pages is checking document.createElement('div').style == null, because if this condition is true, new <style> will have no "type" attribute (nor "style" attribute) and it won't work.

I'll explain document.documentElement.tagName.toLowerCase() === "html" in a while.
Add:

document.documentElement.tagName.toLowerCase() === "html" is a mild condition, Since we need not hide anything in XML view pages, we may choose not to match rules, or we can make uBlock continue to work - which means, we need to fix the bug that content of <style> is shown. Here's a solution found a few days ago:

What we should do is just to use document.createElementNS("http://www.w3.org/1999/xhtml", tagName). A node created by createElementNS has a normal "style", and its children created by setting "innerHTML" also have good "style"s,
Then a normal node with normal "style" won't show its text on the XML page.

@gorhill

Ok, I see:

So your idea seems to work.

Correction: wrong, my mistake, now I see document.documentElement.style !== null for http://sourceforge.net/sitemap.xml.

@gdh1995

Sorry again for my carelessness.

Those snapshots were captured on "/f?kw=%E9%9A%90%E6%9D%80", and I've installed another extension to tweak Chrome's "User Agent", so "tieba.baidu.com" rendered a page for mobile devices. The long long URL is its WAP version.

@gdh1995

document.documentElement.style === null for http://sourceforge.net/sitemap.xml

This is true only when the page is loading, and document.documentElement.style will be a normal CSSStyleDeclaration object when the page is OK. So, I'm not sure if this is a correct condition. Maybe use:

if (document.documentElement.style === null && document.createElement('div').style === null) {
  return;
}

?

@gorhill

To recapitulate:

  • uBlock must not further process XML documents such as http://sourceforge.net/sitemap.xml.
  • uBlock should process XML documents such as http://tieba.baidu.com/mo/q---279....
  • Internally, both are instances of XMLDocument.
  • The only distinguishing features are:
    • document.contentType: respectively text/xml and application/xhtml+xml.
      • However, document.contentType is not a standard property, so best to not use it -- even though it is supported in Chromium.
    • Your suggested solution: document.createElement('div').style is respectively null and not null.
      • I don't like the overhead of creating an element to decide whether uBlock should process the document, however this can be done in a way that this will occur only for XML documents, i.e. when there is an ambiguity. For formal HTMLDocument, the overhead of createElement will never be incurred.
@gdh1995

Em... All the above are right, except that tieba.baidu.com/mo/... is a XHTML document, not a ordinary XML without any special meanings.

I don't like, too, but according to issues in another Chrome extension Vimium, .style == null is a good solution. Here's a same issue on XML pages: philc/vimium#1640 .

May we use your document.documentElement.style === null first and wait for more feedbacks?

@gorhill

May we use your document.documentElement.style === null first

I corrected myself above, on reload, I observed that document.documentElement.style !== null for http://sourceforge.net/sitemap.xml. The createElement solution is fine if it occurs only in case of ambiguity, which is the less common case.

@gdh1995

Maybe we can check document.contentType.indexOf("xml") >= 0 first, which is a less common case, and if the case is not met, then we need not to create a <div> node to check unsupported document types any more.

@gorhill

contentType is non-standard, I prefer to avoid.

Chosen solution is: document.createElement('div') instanceof HTMLDivElement. No need to test for style.

@gdh1995

Oh yes.

@gorhill gorhill added a commit to gorhill/uBlock that referenced this issue
@gorhill gorhill this fixes chrisaljoudi/uBlock#1528
An XML document can be a valid HTML document. Try to instanciate
a HTMLDivElement to find out whether we are dealing with an actual
HTML document or not.
1a380f0
@gorhill gorhill added a commit to gorhill/uBlock that referenced this issue
@gorhill gorhill chrisaljoudi/uBlock#1528: apply fix to scriptlets too 94a1d72
@gorhill gorhill added a commit to gorhill/uBlock that referenced this issue
@gorhill gorhill chrisaljoudi/uBlock#1528: apply fix to element picker 6600a27
@ahmadassaf ahmadassaf added a commit to ahmadassaf/uBlock that referenced this issue
@ahmadassaf ahmadassaf Merge branch 'master' of https://github.com/gorhill/uBlock
* 'master' of https://github.com/gorhill/uBlock: (125 commits)
  brought information up to date
  interim version for dev build
  this fixes #672
  this fixes #678
  translation work from https://crowdin.com/project/ublock
  vAPI.net.registerListeners(): code review
  translation work from https://crowdin.com/project/ublock
  interim version for dev build
  this fixes #668, #669
  this fixes #667
  this fixes #666
  translation work from https://crowdin.com/project/ublock
  this fixes #654
  translation work from https://crowdin.com/project/ublock
  Update README.md
  Update README.md
  Update README.md
  Update README.md
  Update README.md
  chrisaljoudi#1528: apply fix to element picker
  ...
51d9b2c
@gdh1995

Closed.

Sorry, I forgot this issue...

@gdh1995 gdh1995 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.