_isCmdLineEmpty should not clobber window.arguments[0]

RESOLVED FIXED in Firefox 18

Status

()

Firefox
Session Restore
--
minor
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: Gavin, Assigned: mkohler)

Tracking

Trunk
Firefox 18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

It's unexpected that this method would have side-effects, and it doesn't seem to serve a purpose, since by the time this code has run (triggered via delayedStartup), the argument will have already been handled by the window's onload handler.
(Assignee)

Comment 1

6 years ago
Created attachment 645688 [details] [diff] [review]
Patch v1
Assignee: nobody → michaelkohler
Status: NEW → ASSIGNED
Attachment #645688 - Flags: review?(ttaubert)
Comment on attachment 645688 [details] [diff] [review]
Patch v1

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

Thank you for tackling this, Michael.

Unfortunately we can't just remove the second argument and the local 'pinnedOnly' variable. We still need this as "aWindow.arguments[0]" affects the return value. We just shouldn't modify "aWindow.arguments" but rather save this to a local variable.
Attachment #645688 - Flags: review?(ttaubert) → review-
(Assignee)

Comment 3

6 years ago
Created attachment 645789 [details] [diff] [review]
Patch v2
Attachment #645688 - Attachment is obsolete: true
Attachment #645789 - Flags: review?(ttaubert)
Comment on attachment 645789 [details] [diff] [review]
Patch v2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +3889,5 @@
>  
>      if (!pinnedOnly) {
>        let defaultArgs = Cc["@mozilla.org/browser/clh;1"].
>                          getService(Ci.nsIBrowserHandler).defaultArgs;
> +      var windowArguments = aWindow.arguments;

Nit: please use 'let' for variable definitions. Let is the new var!

The actual problem here is that windowArguments does now contain a reference to aWindow.arguments. Modifying windowArguments[0] is therefore the same as modifying aWindow.arguments[0].
Attachment #645789 - Flags: review?(ttaubert)
(Assignee)

Comment 5

6 years ago
Created attachment 646548 [details] [diff] [review]
Patch v3
Attachment #645789 - Attachment is obsolete: true
Attachment #646548 - Flags: review?(ttaubert)
Comment on attachment 646548 [details] [diff] [review]
Patch v3

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

Thanks Michael, great work!
Attachment #646548 - Flags: review?(ttaubert) → review+
This is checkin-needed, right?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ebc0ba34959d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
You need to log in before you can comment on or make changes to this bug.