464754 - nsIContentPolicy.shouldLoad() called without context for scripts

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Wladimir Palant]

This regressed between 2008-11-11-03 and 2008-11-12-03 - now the content policies are sometimes triggered with null as context parameter for scripts. This effectively prevents Adblock Plus from blocking these scripts, other content policies don't deal well with null values in this parameter either.

To reproduce install Adblock Plus 0.7.5.5 from https://addons.mozilla.org/en-US/firefox/addon/1865, go to http://www.ip-secrets.info/trace.php and press Ctrl+Shift+V to open the list of blockable items. You should see a script from bin-layer.de in the list, the page loads it. However, in the current nightly the script won't appear in the list unless the page is loaded from cache. Pressing Ctrl+Shift+R makes sure you don't load the page from cache - nsIContentPolicy.shouldLoad() is still called then but context parameter is null.

Caller is http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#210 and it didn't change recently, the bug must be in the code calling it.

Filing under JS Engine - looking at the list of relevant changes, one of JS Engine changes must have regressed it.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

This is fallout from speculative parsing. The problem is that when we initiate the load, we don't have a context element to pass to the content policy. I'm mostly unfamiliar with content policies, unfortunately. Is it sufficient to pass the window instead for this case? Otherwise, I'm not exactly sure what to do -- we could create a dummy script element or something, but that seems fragile...

Comment 2

[Wladimir Palant]

At the very least the document should be passed - that's what happens for background images for example. I am not really happy with that because it will make unblocking the script without reloading the page impossible (you simply don't know where to find the script node to make it redo the request) but I guess nothing can be done about that. At least that's only a script and not a visual element.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

How expensive is it to call shouldLoad? We could call it twice, once just before we load the script (with the document as the context) and then again once we're about to execute it for real since we'll have the script element by then. But that might be an abuse of the API.

Comment 4

[Boris Zbarsky [:bzbarsky]]

You could call ShouldProcess befre executing; that's what it's for.

It's "somewhat expensive", sadly...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Another option is to just call shouldLoad with the document when preloading.  If the script is not blocked, that's it.  If it is, we'll not preload it, and then hit the normal script-loading codepath and call shouldLoad at that point with the node as context.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note that this would give slightly different behavior than CSS preload, which would do the check both for the preload and for the actual node even if the preload is allowed, but that's ok imo.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This appears to fix it. We will call the content policy checker twice for each preloaded script.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 348096 [details] [diff] [review]
Proposed fix

>+++ b/content/base/src/nsScriptLoader.cpp
>+  nsISupports *context = aRequest->mElement.get()
>+                         ? static_cast<nsISupports *>(aRequest->mElement.get())
>+                         : static_cast<nsISupports *>(mDocument);

I guess we can't use NODE_FROM because aRequest doesn't have the nsIContent offhand, huh?

Looks ok, given that.  You can consider this an r+sr unless you really want jst to also look.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 348096 [details] [diff] [review]
Proposed fix

I'll take the sr.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> I guess we can't use NODE_FROM because aRequest doesn't have the nsIContent
> offhand, huh?

Well, I could static_cast my nsIScriptElement to nsIDOMHTMLScriptElement if you think that's cleaner. I'm not terribly concerned either way.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Nah, the benefit of NODE_FROM would just have been shorter clearer code.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 348096 [details] [diff] [review]
Proposed fix

It would be good to get this into beta 2 so adblock isn't broken.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

There are multiple implementations of nsIScriptElement, so no bad casting please.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #12)
> (From update of attachment 348096 [details] [diff] [review])
> It would be good to get this into beta 2 so adblock isn't broken.

That, IMO, makes this a blocker.

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348096 [details] [diff] [review]
Proposed fix

a191b2=beltzner for good measure, though this is now a b2 blocker

Comment 16

[Ryan Flint [:rflint] (ping via IRC for reviews)]

http://hg.mozilla.org/mozilla-central/rev/9a453249ca6c

Comment 17

[Wladimir Palant]

Verified - fixed.

Comment 18

[Aakash Desai [:aakashd]]

Wladimir, can you add the build Id you tested this against?

Comment 19

[Wladimir Palant]

That would be 3.1b2pre build 2008112003 on Windows XP.