Closed Bug 1104623 Opened 9 years ago Closed 9 years ago

NS_ERROR_DOM_BAD_URI error using worker on url chrome://browser/content/browser.xul

Categories

(Core :: DOM: Workers, defect)

35 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- unaffected
firefox35 + wontfix
firefox36 + fixed
firefox37 + fixed

People

(Reporter: charles.jourdan, Assigned: ckerschb)

References

Details

(Keywords: regression)

Attachments

(2 files, 9 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20141124004001

Steps to reproduce:

Go to the page : chrome://browser/content/browser.xul
Open the devtools to execute this javascript code :
var worker = new Worker(URL.createObjectURL(new Blob(["console.log('Hello world !!');"])));
worker.onerror = function(e){ console.error(e); };


Actual results:

On Firefox stable/Beta, there is no problem but on Firefox Aurora, I get an exception.

Error: Failed to load script (nsresult = 0x805303f4)

Same issue on Firefox nightly ( 36.0a1 (2014-11-24) ).


Expected results:

It should print «Hello world !!» in console like on Firefox stable/Beta.
baku, I thought this was fixed by bug 1094257? (the nightly quoted in comment 0 is significantly later than that fix)
Component: Untriaged → DOM: Workers
Flags: needinfo?(amarchesini)
Product: Firefox → Core
See Also: → 1094257
Summary: Error using worker on url chrome://browser/content/browser.xul → NS_ERROR_DOM_BAD_URI error using worker on url chrome://browser/content/browser.xul
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch blob.patch (obsolete) — — Splinter Review
This is a different issue. The issue here is that we are loading a blob as worker script from a chrome code.
Attachment #8529008 - Flags: review?(bugs)
This is a regression from which bug?
Flags: needinfo?(amarchesini)
good=2014-09-22
bad=2014-09-23
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5bd6e09f074e&tochange=790f41c631cc

suspected:
Christoph Kerschbaumer — Bug 1038756: Callsites creating a channel in /dom/workers/ (r=bent)
Thanks, yes, it seems that the origin of the regression.
Flags: needinfo?(amarchesini)
Comment on attachment 8529008 [details] [diff] [review]
blob.patch

So I don't know why this would be the right thing.
Why do we need this kind of special case for workers?

What is the channelPrincipal here? And if loadPrincipal is system, why 
channelPrincipal isn't?
Attachment #8529008 - Flags: review?(bugs)
Attachment #8529008 - Flags: review-
Attachment #8529008 - Flags: feedback?(mozilla)
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8529008 [details] [diff] [review]
> blob.patch
> 
> So I don't know why this would be the right thing.
> Why do we need this kind of special case for workers?
> 
> What is the channelPrincipal here? And if loadPrincipal is system, why 
> channelPrincipal isn't?

I would like Jonas to have a look at this.
Flags: needinfo?(jonas)
Also CC'ing Tanvi here.
(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8529008 [details] [diff] [review]
> blob.patch
> 
> So I don't know why this would be the right thing.
> Why do we need this kind of special case for workers?

You mean special case using the nullPrincipal in case there is no parentDoc?
See comment:
http://mxr.mozilla.org/mozilla-central/source/dom/workers/ScriptLoader.cpp#120
 
> What is the channelPrincipal here? And if loadPrincipal is system, why 
> channelPrincipal isn't?

The question is, is there a parentDoc? If so then we should get a Document and that document should have the systemPrincipal. If that is not the case, then this is wrong. What code path are we hitting?
Flags: needinfo?(jonas)
> What is the channelPrincipal here? And if loadPrincipal is system, why 
> channelPrincipal isn't?

channelPrincipal is a nsINullPrincipal for a blob URI because of this:
https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#971
More precisely because of

https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#979

That does indeed seem to be the place that we need to fix. But we should look at why that code was put there.

I'd also like to understand why this wasn't a problem before bug 1038756
Comment on attachment 8529008 [details] [diff] [review]
blob.patch

I think Jonas summarized the problem in detail. Clearing the feedback flag for now till we have some more info available.
Attachment #8529008 - Flags: feedback?(mozilla)
Still investigating this issue, but what I see is that the channel of the blob URI doesn't have  the nsILoadInfo::SEC_FORCE_INHERIT_PRINCIPAL flag. So we don't load the 'loadingPrincipal' here:

https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#334
Attached patch bug_1104623.patch (obsolete) — — Splinter Review
Please note that in bug 1087442 we are going to attach the loadInfo in each individual protocol handler. In other words, we pass all the individual loadInfo arguments to nsIOService->NewChannel which then passes the arguments along to the individual protocolhandlers. As an interim solution to fix this regression we have to check whether the channel created by the protocol handler already has a loadInfo attached. In such a case, e.g. for blobs, we have to inherit those security flags. We do this by comparing the securityFlags before we attach the loadInfo in this patch.

Boris, can you take a look at this? If you are too busy I can pass it along to Jonas. Also asking Steve, since he is a necko peer for review.
Attachment #8529008 - Attachment is obsolete: true
Attachment #8532312 - Flags: review?(sworkman)
Attachment #8532312 - Flags: review?(bzbarsky)
Baku, would you mind providing a testcase for this problem? In Bug 1087442 we are modifying the loadInfo again. And probably there are more patches following. Would be great if we have that in a testcase! Any chance I could convince you to write one?
Flags: needinfo?(amarchesini)
Comment on attachment 8532312 [details] [diff] [review]
bug_1104623.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +258,5 @@
> +                          aRequestingNode, aSecurityFlags, aContentPolicyType);
> +  if (!loadInfo) {
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +  channel->SetLoadInfo(loadInfo);

So all this stuff will move into the ioservice?

@@ +292,5 @@
> +                                      aCallbacks,
> +                                      aLoadFlags,
> +                                      aIoService);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  return NS_OK;

You should also call SetLoadInfo here, right? Or is that too going to happen in a different bug?
Comment on attachment 8532312 [details] [diff] [review]
bug_1104623.patch

>+      aSecurityFlags = securityFlags;

I believe this will overwrite the sandboxing flag if that's present in aSecurityFlags.

If this passed try, we need a test that tests a sandboxed iframe loaded from a blob: URI.

Past that, do we want to create a new loadinfo or update the flags in the existing one (e.g. by adding a method on LoadInfo that takes a LoadInfo argument and copies the relevant flag over as needed)?
Attachment #8532312 - Flags: review?(bzbarsky) → review-
(In reply to Jonas Sicking (:sicking) from comment #16)
> Comment on attachment 8532312 [details] [diff] [review]
> bug_1104623.patch
> 
> Review of attachment 8532312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/public/nsNetUtil.h
> @@ +258,5 @@
> > +                          aRequestingNode, aSecurityFlags, aContentPolicyType);
> > +  if (!loadInfo) {
> > +    return NS_ERROR_UNEXPECTED;
> > +  }
> > +  channel->SetLoadInfo(loadInfo);
> 
> So all this stuff will move into the ioservice?

That is correct.

> @@ +292,5 @@
> > +                                      aCallbacks,
> > +                                      aLoadFlags,
> > +                                      aIoService);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  return NS_OK;
> 
> You should also call SetLoadInfo here, right? Or is that too going to happen
> in a different bug?

I had an assertion here, that checked that aLoadInfo->GetSecurityFlags() == loadInfo->GetSecurityFlags(), but that assertion got triggered. I agree, that we want to same loadInfo instance on the channel in that case. But calling setLoadInfo here, we would need to make securityFlags *NOT* readonly, which I was avoiding by not setting the loadInfo here, but rather fix that in Bug 1087442.
(In reply to Boris Zbarsky [:bz] from comment #17)
> Comment on attachment 8532312 [details] [diff] [review]
> bug_1104623.patch
> 
> >+      aSecurityFlags = securityFlags;
> 
> I believe this will overwrite the sandboxing flag if that's present in
> aSecurityFlags.

That means, you would suggest to merge SEC_FORCE_INHERIT_PRINCIPAL if that is set already on the loadinfo?

> If this passed try, we need a test that tests a sandboxed iframe loaded from
> a blob: URI.

I haven't pushed that to try, I was just testing the testcase provided in this bug. I think you are right, we should just merge the flag from the loadInfo already on the channel into aLoadInfo before setting it in the loadinfo.

> Past that, do we want to create a new loadinfo or update the flags in the
> existing one (e.g. by adding a method on LoadInfo that takes a LoadInfo
> argument and copies the relevant flag over as needed)?

I am not sure, what would you suggest? Jonas what do you want? I am fine with either, but lean towards figuring out the appropriate arguments to create a new loadInfo beforehand and pass those arguments to the one constructor we have. But as I said, I don't feel strongly about that. I am open to suggestions.
> merge SEC_FORCE_INHERIT_PRINCIPAL if that is set already on the loadinfo?

Yes, exactly.

If this code is temporary, I don't feel too strongly about new loadinfo vs not.
Attached patch test (obsolete) — — Splinter Review
Here a test for this issue.
Flags: needinfo?(amarchesini)
Attachment #8532577 - Flags: review?(mozilla)
Attached patch bug_1104623.patch (obsolete) — — Splinter Review
Updated the patch to merge the securityFlags instead of overwriting them. I think we want to uplift the attached patch to Aurora to fix the problem there whereas we will refactor those bits in Nightly within the patches in Bug 1087442.
Attachment #8532312 - Attachment is obsolete: true
Attachment #8532312 - Flags: review?(sworkman)
Attachment #8532579 - Flags: review?(sworkman)
Attachment #8532579 - Flags: review?(bzbarsky)
(In reply to Andrea Marchesini (:baku) from comment #21)
> Created attachment 8532577 [details] [diff] [review]
> test
> 
> Here a test for this issue.

Thanks for the test baku. It's not working at the moment though, because the 'document' is not defined.

You can modify your onerror handler to get the debug info:

worker.onerror = function(error) {
  ok(false, "Worker.onerror (" + error.message + ")");
  gBrowser.removeTab(tab);
  finish();
}

I added some debug statements while running your tests. It seems the netUtil code does the right thing.
So probably we just have to tweak the test a little.
Attached patch test (obsolete) — — Splinter Review
Attachment #8532577 - Attachment is obsolete: true
Attachment #8532577 - Flags: review?(mozilla)
Attachment #8532627 - Flags: review?(mozilla)
Comment on attachment 8532627 [details] [diff] [review]
test

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

Thanks Baku - I ran this locally - works now. I think the test is fine, but I think we need a dom/worker-peer to look at this in addition. Jonas can probably do that for us.

::: dom/workers/test/browser_bug1104623.js
@@ +31,5 @@
> +    let worker = new tab.linkedBrowser.contentWindow.Worker(blobURL);
> +    ok(worker, "Worker has been created");
> +
> +    worker.onerror = function() {
> +      ok(false, "Worker.onerror!");

Nit: you could add the error message, which makes it easier to debug in case something goes wrong.
worker.onerror = function(error) {
  ok(false, "Worker.onerror (" + error.message + ")");
Attachment #8532627 - Flags: review?(mozilla)
Attachment #8532627 - Flags: review?(jonas)
Attachment #8532627 - Flags: review+
Attached patch test (obsolete) — — Splinter Review
I don't think we need a review from a worker peer review for this mochitest.
Attachment #8532627 - Attachment is obsolete: true
Attachment #8532627 - Flags: review?(jonas)
Comment on attachment 8532579 [details] [diff] [review]
bug_1104623.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +249,5 @@
> +    aSecurityFlags |= loadInfo->GetSecurityFlags();
> +  }
> +
> +  // create a new Loadinfo with the potentially updated securityFlags
> +  loadInfo = 

nit: trailing whitespace.
Attachment #8532579 - Flags: review?(sworkman) → review+
Comment on attachment 8532579 [details] [diff] [review]
bug_1104623.patch

r=me
Attachment #8532579 - Flags: review?(bzbarsky) → review+
Comment on attachment 8532666 [details] [diff] [review]
test

Re-Setting the r+ flag which got cleared accidentaly.
Attachment #8532666 - Flags: review+
Attached patch bug_1104623.patch (obsolete) — — Splinter Review
Fixed the trailing whitespace, carrying over r+ from bz and sworkman.
To make sure everything is fine, I pushed the change to try as well:
   https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2f5fe9cdd72
Attachment #8532579 - Attachment is obsolete: true
Attachment #8532753 - Flags: review+
Attached patch bug_1104623.patch — — Splinter Review
We forgot to propagate the BaseURI which is now also part of the LoadInfo which caused "viewsource/test/browser/browser_bug464222.js" to fail.

Boris, can you greenlight this again please? I added a C++ friendly version of BaseURI to the *.idl and also propagate the baseURI in nsNetutil.h. Everything else is unchanged from the previous patch.
Attachment #8532753 - Attachment is obsolete: true
Attachment #8533008 - Flags: review?(bzbarsky)
Comment on attachment 8533008 [details] [diff] [review]
bug_1104623.patch

Ah, aBaseURI is optional in the ctor.

r=me, but can you fix the indent of the "readonly attribute nsIURI baseURI;" line in the IDL while you're there?
Attachment #8533008 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #32)

> r=me, but can you fix the indent of the "readonly attribute nsIURI baseURI;"
> line in the IDL while you're there?

Sure can. No need to upload new patches to the bug though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae3b6ea1d85
https://hg.mozilla.org/integration/mozilla-inbound/rev/916dbe7cf99d

This also needs to be uplifted to Aurora once landed.
Target Milestone: --- → mozilla37
Seems this new test cause a new intermittent failure in https://bugzilla.mozilla.org/show_bug.cgi?id=1108526
Backed out until the leaks from bug 1108526 are sorted out.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b034a399704b
Assignee: amarchesini → mozilla
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Target Milestone: mozilla37 → ---
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #36)
> Backed out until the leaks from bug 1108526 are sorted out.

Thanks Ryan!

Andrea, I don't think the patch code itself is causing this leakage - can you take a look at your test again? Any idea what might cause this leakage?
Flags: needinfo?(amarchesini)
Attached patch bug_949706_worker_redirect_tests.patch (obsolete) — — Splinter Review
Looking at the test again, I think baku was just missing an
> executeSoon(finish);
instead of just calling finish.

Anyway, I updated the test and pushed the patch including the updated test to try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=87d45a5d4f30

Should work now!
Attachment #8532666 - Attachment is obsolete: true
Attachment #8533520 - Flags: review+
Attached patch bug_1104623_tests.patch (obsolete) — — Splinter Review
oops - uploaded the wrong patch - here we go!
Attachment #8533520 - Attachment is obsolete: true
Attachment #8533521 - Flags: review+
Clearing the loadInfo since it's not needed anymore.
Flags: needinfo?(amarchesini)
Setting the FORCE_INHERIT flag in the nsHostObjectProtocolHandler seems entirely wrong to me. If protocol handlers start setting loadinfo flags, rather than using the ones that were passed in, that means that we can no longer keep a loadinfo constant across redirects.

So my comment 11 is still how I think we should solve this.

It looks like the systemprincipal check in [1] was added by me when I originally wrote that code. Most likely as an extra safety thing. Removing the systemprincipal check is likely the right way to fix this bug.

However understanding why this started when bug 1038756 was landed, and why it wasn't a problem before, would be helpful.

[1] https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#979
> If protocol handlers start setting loadinfo flags

I thought that was the long-term plan for things like data: and javascript:, so callers don't have to manually check for inheriting protocols?

> that means that we can no longer keep a loadinfo constant across redirects.

Mmm, yes.  We used to mask this flag off when propagating across redirects, I thought...

> However understanding why this started when bug 1038756 was landed

We understand that.  Before bug 1038756 landed, the protocol handler set up a loadinfo that said to inherit the principal.  So the principal was inherited.  Then the changes in bug 1038756 created a new loadinfo based on caller-provided information that was set on the channel, clobbering the protocol-handler-provided loadinfo.  The principal then stopped being inherited, and we started reaching the code comment 11 links to.
(In reply to Boris Zbarsky [:bz] from comment #42)
> > If protocol handlers start setting loadinfo flags
> 
> I thought that was the long-term plan for things like data: and javascript:,
> so callers don't have to manually check for inheriting protocols?
>
> > that means that we can no longer keep a loadinfo constant across redirects.
> 
> Mmm, yes.  We used to mask this flag off when propagating across redirects,
> I thought...

The problem is that if the flags are different between redirects, then it either means that security flags are not constant over the lifetime of a loadinfo, or that the loadinfo instance isn't constant over the lifetime of a fetch.

My thinking was instead to make GetChannelResultPrincipal check flags on the protocol handler for the given URI to see if it inherits, and then return the loadingPrincipal.

I thought that we only ever used the FORCE_INHERIT flag in cases where redirects wouldn't affect if we should inherit. I.e. where we inherit due to some property of the code initiating the load, not due to the uri that we were loading. Hence the name "FORCE".

Seems like the blob: is using it another way which is why I didn't realize we would break it.

> > However understanding why this started when bug 1038756 was landed
> 
> We understand that.  Before bug 1038756 landed, the protocol handler set up
> a loadinfo that said to inherit the principal.  So the principal was
> inherited.  Then the changes in bug 1038756 created a new loadinfo based on
> caller-provided information that was set on the channel, clobbering the
> protocol-handler-provided loadinfo.  The principal then stopped being
> inherited, and we started reaching the code comment 11 links to.

Hum.. the blob: protocol handler was never supposed to "inherit" since we've always been able to deduce the principal using the URI. So there was no need to inherit.

The only exception is for blob: URIs that were created in chrome context where we explicitly choose not to inherit due to the code comment 11 links to.

But it seems like if we actually want these blob: URIs to be treated as having chrome privileges, then we should simply remove that system principal check.

Does that sound reasonable?
> Seems like the blob: is using it another way

It really seems like blob: is basically using it as a workaround for the secman code you linked to.

> Does that sound reasonable?

Yes.  I'm quite happy to do it that way on trunk.  For Aurora, I think the patch in this bug right now, which just reverts to the behavior we used to have, is safest.
Sounds good to me. I.e. use current patch on aurora, and remove the principal check (as well as remove the setting or loadinfo in the hostobjectprotocolhandler) on m-c.
(In reply to Jonas Sicking (:sicking) from comment #45)
> Sounds good to me. I.e. use current patch on aurora, and remove the
> principal check (as well as remove the setting or loadinfo in the
> hostobjectprotocolhandler) on m-c.

That sounds reasonable to me as well. Let's land this patch and uplift to aurora. Then I will incorporate the suggested fix in Bug 1087442 (which is also almost ready to land right after).
I am out of office today, can someone check that in for me please? I fixed the test failure - see Comment 38. Both patches are ready to go. Thanks!
Keywords: checkin-needed
Flags: in-testsuite+
Keywords: checkin-needed
All instances have been on Windows so far.
I think rewriting the test to use store the request rather than use a timer might fix the leak.
Err.. nevermind. I got bugs confused. Please ignore the previous comment.
Attached patch bug_1104623_tests.patch — — Splinter Review
Since the blob is created using createObjectURL(), I think we also have to call revokeObjectURL() [1] when the test completed.

As soon as try opens I will push that to try as well. Backing out a patch three times would be too much.

[1] https://developer.mozilla.org/en-US/docs/Web/API/URL.revokeObjectURL
Attachment #8533521 - Attachment is obsolete: true
Attachment #8534538 - Flags: review+
I retriggered the tests several times, that should work now:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a2146311e4fa
  https://hg.mozilla.org/integration/mozilla-inbound/rev/932daea51104
Target Milestone: --- → mozilla37
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #55)
> I retriggered the tests several times, that should work now:
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2146311e4fa
>   https://hg.mozilla.org/integration/mozilla-inbound/rev/932daea51104

this is still causing problems (same reason as for comment #49 from Ryan) in https://treeherder.mozilla.org/logviewer.html#?job_id=4540947&repo=mozilla-inbound after landing in the new test. So i had to back this out again since this leak is permanent :(
You might need to revoke the URL before closing the tab.
Flags: in-testsuite+ → in-testsuite?
(In reply to Jonas Sicking (:sicking) from comment #57)
> You might need to revoke the URL before closing the tab.

I thought this is what I was doing, but apparently there is an off by one line-error in worker.onmessage. In worker-onerror we did this correctly. That might be it. Thanks Jonas.
Alright, what Jonas said in Comment 57 actually makes sense to me. Trying to land this again on inbound. I haven't pushed it to TRY again because TRY already has been green before (see Comment 55). Sorry for all the trouble this patch is causing, hopefully it works now:

 https://hg.mozilla.org/integration/mozilla-inbound/rev/5b53df2993fa
 https://hg.mozilla.org/integration/mozilla-inbound/rev/c63a0dc3e90f
Flags: in-testsuite? → in-testsuite+
Oooh, I think the blob stuff was a complete red herring. Looking at the code it looks like no matter what, forgetting to revoke a bloburl will only ever leak the blob, not a window.

Looking at the code, it's could be the worker or the "load" event listener that's causing the leak. You should probably unregister the "load" event handler and terminate the worker. And set worker.onmessage to null just to be sure. And please do remove worker.onerror, it just adds complexity while we're tracking this down.


We could check in the fix without checking in the test, because it seems very unlikely that the fix is what's causing the leak and that the bug is in the test (or elsewhere).
(In reply to Jonas Sicking (:sicking) from comment #61) 
> We could check in the fix without checking in the test, because it seems
> very unlikely that the fix is what's causing the leak and that the bug is in
> the test (or elsewhere).

That makes sense, since so many other bugs are kind of blocked by this one.
  https://hg.mozilla.org/integration/mozilla-inbound/rev/eb7d6261b8ce

Setting intestsuite? so we remember we have to check in the testcase. Also setting needinfo-flag for baku. He promised to take a look at the testcase again. Thanks baku!
Flags: needinfo?(amarchesini)
Flags: in-testsuite?
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/eb7d6261b8ce
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Once the test is done and landed on nightly we should not forget to uplift the patch including the test to aurora.
Christoph, could you fill the uplift request to aurora and beta? Thanks
Flags: needinfo?(mozilla)
Comment on attachment 8533008 [details] [diff] [review]
bug_1104623.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1038756 introduced the regression.

[User impact if declined]:
Can't load blobs as worker scripts from chrome code.

[Describe test coverage new/current, TBPL]:
The test got backed out several times, because there is a memory-leak issue in the test itself which caused intermittent errors on windows. Currently the test is going to be remodeled in bug 1110710.

[Risks and why]: 
[String/UUID change made/needed]:
Yes, we changed the uuid of nsILoadInfo.idl.
Flags: needinfo?(mozilla)
Attachment #8533008 - Flags: approval-mozilla-beta?
Attachment #8533008 - Flags: approval-mozilla-aurora?
Comment on attachment 8533008 [details] [diff] [review]
bug_1104623.patch

We can take this on Aurora but to take an idl change in Beta could break add-on compatibility -- can this be done another way for beta?  Alternately can we confirm if addons will be affected?
Flags: needinfo?(jorge)
Attachment #8533008 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Lukas Blakk [:lsblakk] use ?needinfo from comment #67)
> Comment on attachment 8533008 [details] [diff] [review]
> bug_1104623.patch
> 
> We can take this on Aurora but to take an idl change in Beta could break
> add-on compatibility -- can this be done another way for beta?  Alternately
> can we confirm if addons will be affected?

As far as I can see the only change is the addition of a noscript function, which only potentially breaks binary XPCOM compatibility. Since we have very little insight into binary add-ons, I couldn't say for sure if they would break, though my guess is that some will.

I'd favor going with a different solution for beta. While it's early in the cycle, I don't expect add-on devs to be quick to react in the coming weeks.
Flags: needinfo?(jorge)
(In reply to Jorge Villalobos [:jorgev] from comment #69)
> As far as I can see the only change is the addition of a noscript function,
> which only potentially breaks binary XPCOM compatibility. Since we have very
> little insight into binary add-ons, I couldn't say for sure if they would
> break, though my guess is that some will.

Binary compatibility is exactly what beta has to guarantee. If we don't care binary compatibility, we don't have to maintain IID in the first place.
Comment on attachment 8533008 [details] [diff] [review]
bug_1104623.patch

Without an option to maintain binary compatibility, we won't have this fix in 35.
Attachment #8533008 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
QA Whiteboard: [good first verify]
:baku, probably you have just forgotten about this bug since it has been awhile, any chance you could revisit the testcase and try to reland?

Re-opening the bug since we haven't landed the testcase for it yet!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We don't reopen bugs for unlanded testcases.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Christoph, yes, you are right. I completely forgot about this bug. I'll try to work on it this or next week.
Flags: needinfo?(amarchesini)
re-needinfo'ing for a test case.  Or we can file a new bug for that.
Flags: needinfo?(amarchesini)
I found the solution. Just pushed to try. When I have the result, finger-crossed, it's green!
https://treeherder.mozilla.org/#/jobs?repo=try&revision=93e82bad1523
Flags: needinfo?(amarchesini)
Ryan, since the test landed. I think we change the in-testsuite[?] flag, right?
Flags: needinfo?(ryanvm)
Yes
Flags: needinfo?(ryanvm)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.