Closed Bug 475141 Opened 11 years ago Closed 11 years ago

Create a C++ wrapper for the private browsing service for Firefox 3.1

Categories

(Firefox :: Private Browsing, defect, P2, critical)

defect

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

(Keywords: dev-doc-complete, fixed1.9.1)

Attachments

(1 file)

If bug 360207 does not get fixed by the time Firefox 3.1 is going to be shipped, we need to create a C++ wrapper for the private browsing service which pushes a null js context onto the js context stack before each call to the real js service.

Bug 472396 is a sample of the problems which can happen in this case, and it includes a patch to do this wrapping for the satchel component.

If bug 360207 is fixed in time for the 3.1 release, and we can show that the problem in bug 472396 does not happen with that fix, this can be WONTFIXed.
Flags: wanted-firefox3.1?
This needs to block unless we fix bug 360207.  The current setup is very fragile and likely to explode in our faces with no provocation...
Flags: blocking-firefox3.1?
Flags: wanted-firefox3.1?
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1+
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
Blocks: 468877
Ehsan: ETA for this?
(In reply to comment #2)
> Ehsan: ETA for this?

I'll have a patch here by the end of this week.
Whiteboard: [PB-P1]
Attached patch Patch (v1)Splinter Review
This patch defines a new service in C++ which is a wrapper to nsPrivateBrowsingService, with contract ID "@mozilla.org/privatebrowsing-wrapper;1", and swicthes the definition of NS_PRIVATE_BROWSING_SERVICE_CONTRACTID in nsNetCID.h to this new contract ID, so that all in-tree C++ consumers automatically switch to this wrapper component.

It also undoes the changes made by bug 472396 to the satchel component, because now the JS context pushing is handled by the service itself, and it happens transparently to the clients.

I also ported all of the unit tests for the PrivateBrowsing service to the new wrapper service and made sure that all in-tree tests related to private browsing pass with this patch.

mconnor, it would be great if you can review this soon so that we can land this for Beta 3 to get some testing from the community.  Also, if you feel that a simple C++ test is also required for the wrapper component, please let me know.
Attachment #362458 - Flags: superreview?(mconnor)
Attachment #362458 - Flags: review?(mconnor)
Whiteboard: [PB-P1]
Comment on attachment 362458 [details] [diff] [review]
Patch (v1)

I'll try to get to this on Monday.
Whiteboard: [has patch][needs review mconnor]
Comment on attachment 362458 [details] [diff] [review]
Patch (v1)

Looks good, really like the tests stuff, though I wish there was a nicer way of diffing that type of thing so the diff looked less huge so I would have reviewed this sooner. :)
Attachment #362458 - Flags: superreview?(mconnor)
Attachment #362458 - Flags: superreview+
Attachment #362458 - Flags: review?(mconnor)
Attachment #362458 - Flags: review+
Whiteboard: [has patch][needs review mconnor] → [has patch][has reviews]
http://hg.mozilla.org/mozilla-central/rev/dcb3aa2fd0ab
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Keywords: dev-doc-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
Target Milestone: Firefox 3.1 → Firefox 3.2a1
I backed this out on suspicion of causing semi-random mochichrome orange (which did indeed clear up).  Please reland on its own at some point, or at least not with the other two patches it landed with.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 362458 [details] [diff] [review]
Patch (v1)

+  nsCOMPtr<nsIJSContextStack> jsStack;
+  nsresult rv = PrepareCall(getter_AddRefs(jsStack));
+  NS_ENSURE_SUCCESS(rv, rv);
+  rv = mPBService->GetPrivateBrowsingEnabled(aPrivateBrowsingEnabled);
+  FinishCall(jsStack);

Sorry for giving drive-by review comments, but please please write a C++ class that does the prepare/finish in its ctor/dtor so that it's impossible to forget to finish here. Doing so would cause security problems, so let's not leave that door open.

Please do that before re-landing this.
(In reply to comment #9)
> I backed this out on suspicion of causing semi-random mochichrome orange (which
> did indeed clear up).  Please reland on its own at some point, or at least not
> with the other two patches it landed with.

Hmm, Boris am I missing something?  I don't see the relevant changesets neither on m-c nor on 1.9.1...
(In reply to comment #11)
> Hmm, Boris am I missing something?  I don't see the relevant changesets neither
> on m-c nor on 1.9.1...

Never mind, I was looking around the time that the comment was posted...

Do you want me to back out the branch patch as well?
bz: please see comment 12.
(In reply to comment #10)
> Sorry for giving drive-by review comments, but please please write a C++ class
> that does the prepare/finish in its ctor/dtor so that it's impossible to forget
> to finish here. Doing so would cause security problems, so let's not leave that
> door open.
> 
> Please do that before re-landing this.

Sure, will do.
If branch tboxes aren't being orange, probably no need to back out on branch.
Re-landed on trunk with jst's comments addressed: <http://hg.mozilla.org/mozilla-central/rev/d6d53dac9f99>
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Is this going into Firefox 3.5?  The target milestone says 3.6a1, but it's keyworded as fixed in 1.9.1, so I'm confused. :)
I brought up the same thing in 460343.  I always thought it was the first release that will include the feature.  Using the trunk to mark the milestone just seems weird once the fixed has actually been checked in.
(In reply to comment #17)
> Is this going into Firefox 3.5?  The target milestone says 3.6a1, but it's
> keyworded as fixed in 1.9.1, so I'm confused. :)

Yes, this has been landed on 1.9.1 so it will be included in Firefox 3.5.
(In reply to comment #18)
> Using the trunk to mark the milestone
> just seems weird once the fixed has actually been checked in.

See http://groups.google.ca/group/mozilla.dev.planning/browse_thread/thread/df008ff78bbec940/74f4d087edfccdb5 .
How important is this to document? I ask because the patch is large and will take time to wade through, and it seems after a brief read through to be the sort of thing that's of interest to a relatively small subset of people.
Eric, I believe all that needs documenting here is that C++ consumers of nsIPrivateBrowsingService must get it using the "@mozilla.org/privatebrowsing-wrapper;1" contract id instead of the "@mozilla.org/privatebrowsing;1" contract id JS consumers may (and should) use.
Added a note to that effect near the top of the nsIPrivateBrowsing article:

https://developer.mozilla.org/En/NsIPrivateBrowsingService
Thank you Sheppy.  The article looks great, and there is nothing more to document about this bug there.
You need to log in before you can comment on or make changes to this bug.