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.

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...

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.

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.

You could call ShouldProcess befre executing; that's what it's for. It's "somewhat expensive", sadly...

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.

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.

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

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 on attachment 348096 [details] [diff] [review] Proposed fix I'll take the sr.

(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.

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

Comment on attachment 348096 [details] [diff] [review] Proposed fix It would be good to get this into beta 2 so adblock isn't broken.

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

(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 on attachment 348096 [details] [diff] [review] Proposed fix a191b2=beltzner for good measure, though this is now a b2 blocker

Verified - fixed.

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

That would be 3.1b2pre build 2008112003 on Windows XP.