Closed Bug 1016875 Opened 5 years ago Closed 5 years ago

URL that returns a non-text/html type with HTML Imports causes crash

Categories

(Core :: DOM: Core & HTML, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- unaffected
firefox32 --- affected

People

(Reporter: freddyb, Assigned: gkrizsanits)

References

()

Details

(4 keywords, Whiteboard: data: url testcase in comment 0)

Crash Data

Attachments

(10 files, 6 obsolete files)

1.88 KB, text/plain
Details
1.49 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.08 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.03 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
6.44 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.35 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
4.14 KB, patch
mrbkap
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review
1.66 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.22 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.12 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
Attached file output.txt
When supplying a view-source URL as import source, Nightly just crashes.

The test case is simple enough to fit in a data URI, just copy&paste into a Nightly build with the webcomponents pref enabled:

data:text/html,<link id="viewSourceWithCORSDisabled" rel="import" href="view-source:http://example.com">

I have also run this in an address sanitizer build. The console output is attached as output.txt
As a recommendation on how to fix this: Can we please just disallow all non-HTTP/HTTPs schemes? :)
I'm not sure how far the recommendation goes with data URIs and Blob URIs though...
Assignee: nobody → gkrizsanits
FWIW I get the same error with ftp:, resource:, file:, jar: and about: URIs.
Crash Signature: https://crash-stats.mozilla.com/report/index/e46cb74b-27b8-4d3d-9d4b-34db52140528
bp-e46cb74b-27b8-4d3d-9d4b-34db52140528 is not a signature, but a report. :)
Crash Signature: https://crash-stats.mozilla.com/report/index/e46cb74b-27b8-4d3d-9d4b-34db52140528 → [@ mozilla::dom::ImportLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) ]
Summary: view-source URL with HTML Imports causes crash → non-HTTP URL with HTML Imports causes crash
I also get a crash when I import a gif.
Possibly the same bug (crash report at https://crash-stats.mozilla.com/report/index/f8d01afa-f16a-42fe-94f4-2950c2140528)?
The crashes are null derefs so probably not exploitable.

We should not allow loading URLs that are not allowed in an iframe src= at the very least and we should enforce that the thing we loaded had a content-type of text/html -- no sniffing allowed in new specs! If we're crashing because we've already flagged these things as errors then great; I only bring it up in case fixing the crash means that we'd then load these invalid things.
Group: core-security
Whiteboard: data: url testcase in comment 0
Version: 31 Branch → 32 Branch
This is for HTML Imports (webcomponents) a new feature, not iframes. So I guess this is not a regression?
It's a regression in the sense that a web page that used to not crash now crashes.  The only good thing is this is preffed off by default.  ;)

The handling of ImportLoader::mChannel looks totally bogus to me.  For example, it completely ignores the possibility of HTTP redirects, which would cause the request passed to the callbacks to not match what NS_NewChannel returned.  That makes debugging the rest of what happens here extra fun.

The actual bug here is that the code in OnStopRequest is assuming mDocument is not null, aka that the load didn't fail.  There is no particular reason that should be the case.  In fact, OnStartRequest fails out the load if the type is not text/html, which is what causes the crash in this bug: OnStartRequest fails the load and doesn't create a document, OnStopRequest doesn't check the load status and tries to use the document.

As long as we're here:

1)  There is no checking for HTTP error pages (should those still be loaded as the
    import?).
2)  Why does the import document share the main document's loadgroup instead of using its
    own that's placed in the import parent's loadgroup?  Maybe this is sensible, but just
    want to make sure that was a conscious decision.
3)  Why does ImportLoader CC mDocument but not mImportParent?

In general, I assume you did read over the existing code in nsExternalResourceMap::PendingLoad that does similar-but-not-identical things while writing this, right?
Incidentally that means we have no tests for the non-text/html behavior.
Summary: non-HTTP URL with HTML Imports causes crash → URL that returns a non-text/html type with HTML Imports causes crash
Just to make this clear, this is just an initial state for the imports, there are two reasons for it to be on mc, one is to save it from bit-rotting the other is to let some GAIA folks play with it... Anyway, these are are great points so I'm very thank full for looking into this.

(In reply to Boris Zbarsky [:bz] from comment #7)
> 1)  There is no checking for HTTP error pages (should those still be loaded
> as the
>     import?).

No, that part is to be implemented. That part in general feels a bit underspecified to me...
But maybe I just looked over something, in any case, it has to be worked out and implemented.

> 2)  Why does the import document share the main document's loadgroup instead
> of using its
>     own that's placed in the import parent's loadgroup?  Maybe this is
> sensible, but just
>     want to make sure that was a conscious decision.

Semi-conscious... an import can have multiple parents in which case it's hard to tell in which group it should be placed... I might be wrong about this, so it's a very valid question, I just _feel_ like this is the right thing to do which gives me very little comfort... What would be the advantage/disadvantage of not sharing the main document's loadgroup exactly? 

> 3)  Why does ImportLoader CC mDocument but not mImportParent?

mImportParent is just set temporarily until the import is loaded (or failed), then it's nulled out. We _could_ CC it but we decided it's not necessary. If I have not added a comment about it I should. 
(actually I just recalled it, it was in the CC list with a comment, and then when I removed it the comment went with it... will add back the comment at least)

> 
> In general, I assume you did read over the existing code in
> nsExternalResourceMap::PendingLoad that does similar-but-not-identical
> things while writing this, right?

Yes, it's kind of a mixture of that and XHR... And I'm sure a lot of important things are still missing. I barely made the right case scenario to work, there are a lot of polishing needed around the import loading.

(In reply to Boris Zbarsky [:bz] from comment #8)
> Incidentally that means we have no tests for the non-text/html behavior.

This is the best sum of the current case in general. It's time to write a bunch of tests, and the hardest part about it to me is to find a good list of things that should be tested. So this bug is already incredibly useful for me.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> an import can have multiple parents

Just to clarify this part a bit, mImportParent points to the parent that should matter in the script execution order calculation, and it might change as more link imports found by the parsers referring to the same import.
> in which case it's hard to tell in which group it should be placed

The group of the main document, obviously.  ;)

> What would be the advantage/disadvantage of not sharing the main document's loadgroup
> exactly? 

One advantage would be the ability to stop loads for all of an import's subresources without stopping loads for the rest of the subresources for the main document.

Another is that addons won't grab a docshell from your loadgroup and assume you're the document rendering in that docshell; that was the motivation behind the separate loadgroups for SVG resource documents.

> mImportParent is just set temporarily until the import is loaded (or failed)

If that's the intent, it's not implemented correctly.  For example, if AsyncOpen() on the channel fails you will hold on to it forever (as well as blocking scripts forever, actually).  Also, it relies on DOMContentLoaded firing, and it's not clear to me that anything 100% guarantees that.  I'd be less worried if we fixed the AsyncOpen issue and explicitly did cleanup in OnStopRequest, since that's the one thing we _are_ guaranteed will happen if AsyncOpen succeeded.  At least modulo broken extension protocol implementations.
(In reply to Boris Zbarsky [:bz] from comment #11)
> > in which case it's hard to tell in which group it should be placed
> 
> The group of the main document, obviously.  ;)

Right so you mean creating loadGroups for each import but add them all to the masters? Because at the beginning we have no info about the import graph... So either go with the idea (everything to the masters group approach), or we have to be able to move loadGroups around when it turns out that there is another parent document for it then the one that initiated its loading...

Anyway, stopping the import's subresources shouldn't be needed according to Anne. Currently imports are defined as external resources of the main document pretty much, so I kind of agree with him.

The addon's grabbing a docshell part is... I honestly have no idea.

> > mImportParent is just set temporarily until the import is loaded (or failed)
> 
> If that's the intent, it's not implemented correctly.  For example, if
> AsyncOpen() on the channel fails you will hold on to it forever (as well as
> blocking scripts forever, actually).

Will fix that.

>  Also, it relies on DOMContentLoaded
> firing, and it's not clear to me that anything 100% guarantees that.  I'd be
> less worried if we fixed the AsyncOpen issue and explicitly did cleanup in
> OnStopRequest, since that's the one thing we _are_ guaranteed will happen if
> AsyncOpen succeeded.  At least modulo broken extension protocol
> implementations.

Thank you, I will do that. About the event firing, the idea is that I add a timeout
like in XHR. The spec is super vague about this part, and leaves it up to UA to decide
what to do. I'm not big fan of timeouts but don't have any better idea either... :(
> Right so you mean creating loadGroups for each import but add them all to the masters? 

Yes, that's the only viable alternative to just putting everything directly in the master loadgroup.

> Currently imports are defined as external resources of the main document pretty much

OK.  And we don't fire separate observable load events on the import documents themselves or anything, so don't need to track their subresource loads separately?

> I'm not big fan of timeouts but don't have any better idea either

I wouldn't worry about it.
(In reply to Boris Zbarsky [:bz] from comment #13)
> OK.  And we don't fire separate observable load events on the import
> documents themselves or anything, so don't need to track their subresource
> loads separately?
> 

"The link element fires a simple event called load for successful loading attempt. "- what we have in the spec... We actually fire this event after the import document's DOMContentLoaded event, which might be not the right thing to do, now that I think about it... I have never put a lot of thoughts about images and such in an import document since we don't render them anyway, stylesheets can be important though and maybe iframes too... Load event on the import document itself, is actually a very good question, I _think_ we have to fire them. Does that mean they need their own loadGroup?
If you need to separately keep track of the loading state of the import document and its subresources, then you want them to have a separate loadgroup to track that state.  How are you going to track it otherwise?
So I'm trying to figure out how to do the redirection part correctly. Other than stop assuming that in the callback I get the same request that I store in mChannel, do I need to do any specific checks? Just to clarify I'm trying to follow redirections as http://fetch.spec.whatwg.org/#atomic-http-redirect-handling

Also, this w3c bug should be linked here I think: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25924
> do I need to do any specific checks?

At that point, can't you just stop storing mChannel altogether?
(In reply to Boris Zbarsky [:bz] from comment #17)
> At that point, can't you just stop storing mChannel altogether?

Yepp, that's my intention :) Just don't know how cautious should I be with calling SetOwner on it... (other than checking for error after call, which I seem to forgot to do in my patch)
> Just don't know how cautious should I be with calling SetOwner on it...

You want to call SetOwner() on the actual channel that the resulting document will use, no?
(In reply to Boris Zbarsky [:bz] from comment #19)
> You want to call SetOwner() on the actual channel that the resulting
> document will use, no?

Yes, for sure. My worry might be totally meaningless, I was just reading some code related to redirection/channels today and found this: http://mxr.mozilla.org/mozilla-central/source/content/base/src/EventSource.cpp#374

I'm not sure what can be the result of giving a channel a system principal, and if we should do some guarding for this case or not...
> I'm not sure what can be the result of giving a channel a system principal

The resulting document has the system principal.

> and if we should do some guarding for this case or not

Good question.  What should happen with imports in system pages?
(In reply to Boris Zbarsky [:bz] from comment #21)
> Good question.  What should happen with imports in system pages?

In that case I think the only sane thing we could do is to prevent chrome documents to import anything from non-chrome locations... Which I'm not sure if easy to do. We have to be able to tell if an URI is chrome, I'm sure we have some API for that... and then I _think_ it's enough to check it at the very beginning if the to-be-imported URI is chrome or not. Or is there any situation where we might have to deal with redirection or any other tricks in the chrome case? I think I will have to prevent redirection to data uris anyway, so I will have to check if redirection has occurred or not, I might as well add an extra check to prevent redirection entirely in the chrome case just in case...
> Which I'm not sure if easy to do.

Stepping back for a sec, what do we do with chrome that has <script src="somewhere">? Seems like the threat model here is exactly the same.
(In reply to Boris Zbarsky [:bz] from comment #23)
> > Which I'm not sure if easy to do.
> 
> Stepping back for a sec, what do we do with chrome that has <script
> src="somewhere">? Seems like the threat model here is exactly the same.

Hmmm... I will look into it, it's a good starting point, thanks. I have to go now, so I will get back to this tomorrow.
Interesting. The worker script loader does something similar what I was planning to do: http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#533

But the regular script element behavior depends on the CORS check: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#1493

if there was no CORS check we seem like running the script with the channels principal. If there was CORS check (which should be the case for imports...) then it seems like we do nothing.

Import documents share the browser context of the master, so we cannot just import a non-chrome document, I feel like we have to do something like the worker code does.
I could use some help with channel redirection...

I think I will have to implement nsIChannelEventSink::asyncOnChannelRedirect to be able to cancel some redirections (it's not yet clear what to cancel, but redirection from some site to a file on my hard drive probably should not be allowed for example...)

What I don't know is how the redirection happen exactly (without asyncOnChannelRedirect for now). I async open a channel and waiting for OnStartRequest/OnStopRequest/OnDataAvailable calls. IF the redirection occurs, will I get these calls only with the new (post redirection) channel, or some of them will be called with the old one (the channel I opened) too? Or only with the old one and for the new channel to call these methods I have to do some registration in the asyncOnChannelRedirect? (it does not seem like it, but I could have missed something)

Boris, could you help me out or point me to some code / people I should bother with my questions?
Flags: needinfo?(bzbarsky)
> IF the redirection occurs, will I get these calls only with the new (post redirection)
> channel

Yes.  Note that this is actually documented in nsIChannel.idl in the docs for asyncOpen.

I'm happy to answer necko questions in general; Honza Bambas (:mayhemer) is the expert on redirection stuff if you end up having to dig deep into it for some reason.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #27)
> Yes.  Note that this is actually documented in nsIChannel.idl in the docs
> for asyncOpen.

Exactly what I was looking for :) Thanks!
Just to be future proof I added mImportParent to the CC list. It is too simple to make a mistake and end up leaking mImportParent if we don't.
Attachment #8448793 - Flags: review?(mrbkap)
Attached patch part2: timeout for imports. v1 (obsolete) — Splinter Review
The default timeout is a question... since the spec does not give any guideline I wouldn't spend a lot of time on it for now. Later this part should be polished probably, the number should depend on the subimports OR there should be an API for changing it like for XHR, but then it should be done on spec level.
Also this is a slow test :(
mChannel cannot be removed because we need to cancel it during timeout, but it is only used for there (and in Open but that should not be an issue)
Attachment #8448795 - Flags: review?(mrbkap)
Attachment #8448795 - Flags: feedback?(bzbarsky)
Attached patch part3: Redirection support. v1 (obsolete) — Splinter Review
I'm not sure if this patch is review ready... it's kind of a best bet from my side, am I missing anything?
Attachment #8448797 - Flags: feedback?(mrbkap)
Attachment #8448797 - Flags: feedback?(bzbarsky)
Attachment #8448799 - Flags: review?(mrbkap)
Attachment #8448799 - Flags: feedback?(bzbarsky)
Attachment #8448803 - Flags: review?(mrbkap)
Attachment #8448805 - Flags: review?(mrbkap)
Attachment #8448805 - Flags: feedback?(bzbarsky)
Attachment #8448808 - Flags: review?(mrbkap)
Comment on attachment 8448795 [details] [diff] [review]
part2: timeout for imports. v1

Why do we want a timeout by default?  This seems like it would make any tests involving imports the stuff of sheriffs' nightmares....
Attachment #8448795 - Flags: feedback?(bzbarsky) → feedback-
Comment on attachment 8448797 [details] [diff] [review]
part3: Redirection support. v1

How about just passing "this" to NS_NewChannel like you're supposed to if you know the callbacks up front? ;)  Then you don't need mNotificationCallbacks either.

> +  // XXX: should we call CheckMayLoad here?

Aren't you supposed to be using CORS?  I don't see it anywhere here, but if you were it would handle that for you, no?  It's a bit more complicated than just CheckMayLoad.

>\ No newline at end of file

Several of those in the patch; should fix.
Attachment #8448797 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8448799 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8448805 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #36)
> Comment on attachment 8448795 [details] [diff] [review]
> part2: timeout for imports. v1
> 
> Why do we want a timeout by default?  This seems like it would make any
> tests involving imports the stuff of sheriffs' nightmares....

Yeah... That's a totally valid point, just I don't know what else can we do? If a site has
a link import (maybe from another server) and it turns out to be hanging, the script of the
site is blocked until the import is finished loading (and executing its scripts) which might
never happen... and the site won't have any chance to recover. My point is that never giving up on
an imports is bad too and I have no idea how to address the problem. I'm open for suggestions what should we do...

(In reply to Boris Zbarsky [:bz] from comment #37)
> Comment on attachment 8448797 [details] [diff] [review]
> part3: Redirection support. v1
> 
> How about just passing "this" to NS_NewChannel like you're supposed to if
> you know the callbacks up front? ;)  Then you don't need
> mNotificationCallbacks either.
> 
> > +  // XXX: should we call CheckMayLoad here?
> 
> Aren't you supposed to be using CORS?  I don't see it anywhere here, but if
> you were it would handle that for you, no?  It's a bit more complicated than
> just CheckMayLoad.
> 

Yes I should do that... so here is how I think this should look like based on this comment
and some code I read (I might be completely wrong...):
- creating a channel with |this| (not sure if |this| is needed if I use the CORSListenerProxy)
- creating a nsCORSListenerProxy with |this| and then Init it with channel
- AsyncOpen with the CORSListenerProxy
- after this I only have to call the nsIAsyncVerifyRedirectCallback argument in AsyncOnChannelRedirect
the rest should be taken care for me by the proxy

In this case maybe I should use some helper class instead of implementing nsIChannelEventSink
Flags: needinfo?(bzbarsky)
> If a site has a link import (maybe from another server) and it turns out to be hanging,
> the script of the site is blocked until the import is finished loading (and executing its
> scripts) which might never happen

How is that different from the <script> situation?

> - creating a channel with |this| (not sure if |this| is needed if I use the
> CORSListenerProxy)

It is.

> after this I only have to call the nsIAsyncVerifyRedirectCallback argument in
> AsyncOnChannelRedirect

And update mChannel, no?
Flags: needinfo?(bzbarsky)
That's assuming you still want an mChannel, of course.
(In reply to Boris Zbarsky [:bz] from comment #39)
> How is that different from the <script> situation?

Maybe it will be a more common scenario than with scripts... Importing a UI element from an external place. But conceptually nothing, so probably this is only a problem in my mind then, I can just throw away the whole timeout idea.

(In reply to Boris Zbarsky [:bz] from comment #40)
> That's assuming you still want an mChannel, of course.

I think I can get rid of mChannel if we don't need timeout.
> Importing a UI element from an external place

People use <script> for that now.

> I think I can get rid of mChannel if we don't need timeout.

Let's do that.  Then you don't need to implement nsIChannelEventSink or nsIInterfaceRequestor and don't need to set yourself as the callbacks on the channel.
Attachment #8448793 - Flags: review?(mrbkap) → review+
Comment on attachment 8448795 [details] [diff] [review]
part2: timeout for imports. v1

Review of attachment 8448795 [details] [diff] [review]:
-----------------------------------------------------------------

As bz pointed out, we shouldn't do the automatic timeout thing, but there were a couple of hunks that seemed unrelated to the patch as a whole (and one that definitely needs to be landed).

::: content/base/src/ImportManager.cpp
@@ +134,5 @@
>      return nsContentUtils::DispatchTrustedEvent(mNode->OwnerDoc(),
>                                                  mNode,
>                                                  mSuccess ? NS_LITERAL_STRING("load")
>                                                           : NS_LITERAL_STRING("error"),
> +                                                /* aCanBubble = */ false,

This change seems misplaced in this patch. Why make it?

@@ +312,5 @@
>  
> +  if (!mDocument) {
> +    // If at this point we don't have a document, then the error was
> +    // already reported.
> +    return NS_ERROR_FAILURE;

This hunk should live on and get landed (see bug 1033443).
Attachment #8448795 - Flags: review?(mrbkap) → review-
Attachment #8448799 - Flags: review?(mrbkap) → review+
Comment on attachment 8448803 [details] [diff] [review]
part5: test for blob and data imports

Review of attachment 8448803 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/test_imports_nonhttp.html
@@ +16,5 @@
> +  //<![CDATA[
> +    SimpleTest.waitForExplicitFinish();
> +    var counter = 0;
> +    function loaded() {
> +      if ( counter++ == 1 ) {

Odd spacing.

@@ +36,5 @@
> +    var encoded = btoa(stringDoc);
> +    var dataurl = "data:text/html;base64," + encoded;
> +    link.setAttribute("href", dataurl);
> +    link.setAttribute("onload", "loaded()");
> +    link.setAttribute("onerror", "failed()");

Why not set link.onload directly or use addEventListener?
Attachment #8448803 - Flags: review?(mrbkap) → review+
Comment on attachment 8448805 [details] [diff] [review]
part6: System documents should not import non-system documents. v1

Review of attachment 8448805 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ImportManager.cpp
@@ +346,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
>    nsCOMPtr<nsIPrincipal> principal = sop->GetPrincipal();
> +  if (nsContentUtils::IsSystemPrincipal(principal)) {

Shouldn't this be a call to nsIScriptSecurityManager::CheckURIWithPrincipal, similar to <http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp?rev=ed5afdaaa6d3#1013>?
Attachment #8448805 - Flags: review?(mrbkap) → review-
Comment on attachment 8448797 [details] [diff] [review]
part3: Redirection support. v1

Review of attachment 8448797 [details] [diff] [review]:
-----------------------------------------------------------------

If I'm reading the bug correctly, there's a new patch coming here, right?
Attachment #8448797 - Flags: feedback?(mrbkap)
Comment on attachment 8448808 [details] [diff] [review]
part7: GetPrincipal for imports. v1

Review of attachment 8448808 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the weak ref (and corresponding changes -- we don't need to keep refcounting everywhere.

::: content/base/src/ImportManager.h
@@ +123,5 @@
>    // on the import parent document.
>    void BlockScripts();
>    void UnblockScripts();
>  
> +  already_AddRefed<nsIPrincipal> GetPrincipal();

This can return a weak ref.
Attachment #8448808 - Flags: review?(mrbkap) → review+
> Shouldn't this be a call to nsIScriptSecurityManager::CheckURIWithPrincipal,

No, though we need that too, in Open()!
(In reply to Blake Kaplan (:mrbkap) from comment #43)
> >      return nsContentUtils::DispatchTrustedEvent(mNode->OwnerDoc(),
> >                                                  mNode,
> >                                                  mSuccess ? NS_LITERAL_STRING("load")
> >                                                           : NS_LITERAL_STRING("error"),
> > +                                                /* aCanBubble = */ false,
> 
> This change seems misplaced in this patch. Why make it?

Because otherwise if an error event is dispatched and the callback does not cancel it explicitly test will break (since the even bubbles up further). I tried to find any guidelines when should an event bubble and when not, but couldn't... So I would really like to hear some input about this. Spec only say "simple event" does that specify it?  

> 
> @@ +312,5 @@
> >  
> > +  if (!mDocument) {
> > +    // If at this point we don't have a document, then the error was
> > +    // already reported.
> > +    return NS_ERROR_FAILURE;
> 
> This hunk should live on and get landed (see bug 1033443).

I will add this as a separate patch and land it with r=you then. Sorry for the lag here, I had an unexpected PTO because I didn't have my notebook charger with me in Hungary...
Probably I could just flag this r+ but better safe than sorry...
Attachment #8448795 - Attachment is obsolete: true
Attachment #8456196 - Flags: review?(mrbkap)
Attachment #8456196 - Flags: review?(mrbkap) → review+
Attachment #8448797 - Attachment is obsolete: true
Attachment #8456200 - Flags: review?(mrbkap)
It's just a rebased version.
Attachment #8448799 - Attachment is obsolete: true
Attachment #8456203 - Flags: review+
I think we still need this patch...
Attachment #8448805 - Attachment is obsolete: true
Attachment #8456205 - Flags: review?(mrbkap)
Updated version of the already r+ one.
Attachment #8448808 - Attachment is obsolete: true
Attachment #8456207 - Flags: review+
Attachment #8456202 - Flags: feedback?(bzbarsky)
Attachment #8448803 - Attachment is obsolete: true
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #55)
> Created attachment 8456202 [details] [diff] [review]
> Part6: CORS and other security checks for imports. v1

Weird... the CORS part seems to break data URIs for some reason... regular URI's and even blob works fine. Is there a reason why nsCrossSiteListenerProxy should not work when the channel belongs to a dataURI? I get things like this on my console:

247	  rv = corsListener->Init(channel);
(gdb) n
[4151] WARNING: CacheIndex::SetupDirectoryEnumerator() - Entries directory doesn't exist!: file /home/gabor/development/imports/netwerk/cache2/CacheIndex.cpp, line 2515
ERROR: GetDiskSpaceAvailable: STATFS call FAILED. 
[4151] WARNING: 'NS_FAILED(rv)', file /home/gabor/development/imports/netwerk/cache2/CacheFileIOManager.cpp, line 2575
[4151] WARNING: NS_ENSURE_TRUE(http) failed: file /home/gabor/development/imports/content/base/src/nsCrossSiteListenerProxy.cpp, line 859
248	  NS_ENSURE_SUCCESS_VOID(rv);
>+  rv = corsListener->Init(channel);

Might want to pass "true" for the optional aAllowDataURI argument to solve you issue from comment 59.  Also, boo optional boolean arguments.
Comment on attachment 8456202 [details] [diff] [review]
Part6: CORS and other security checks for imports. v1

Why ALLOW_CHROME for the CheckLoadURI call?  I don't think we want that.

Apart from that and the data: thing, looks reasonable.
Attachment #8456202 - Flags: feedback?(bzbarsky) → feedback+
Attachment #8456197 - Flags: review?(mrbkap) → review+
Comment on attachment 8456198 [details] [diff] [review]
Part4: Removing mChannel in ImportLoader. v1

Review of attachment 8456198 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/ImportManager.cpp
@@ +258,1 @@
>                                                         aStream, aOffset,

This indentation looks odd to me.
Attachment #8456198 - Flags: review?(mrbkap) → review+
Attachment #8456200 - Flags: review?(mrbkap) → review+
Comment on attachment 8456202 [details] [diff] [review]
Part6: CORS and other security checks for imports. v1

Review of attachment 8456202 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with bz's comments addressed.
Attachment #8456202 - Flags: review?(mrbkap) → review+
Attachment #8456205 - Flags: review?(mrbkap) → review+
sorry had to backout the last part for bustage on linux like https://tbpl.mozilla.org/php/getParsedLog.php?id=43929466&tree=Mozilla-Inbound
(In reply to Carsten Book [:Tomcat] from comment #65)
> sorry had to backout the last part for bustage on linux like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=43929466&tree=Mozilla-
> Inbound

Thanks for your help! I did a full try run this time: https://tbpl.mozilla.org/?tree=Try&rev=8a0860f22b9d
You need to log in before you can comment on or make changes to this bug.